<HTML><BODY>Please look at the new version.<br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Среда, 28 марта 2018, 12:25 +03:00 от Vladimir Davydov <vdavydov.dev@gmail.com>:<br><br><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15222291220000000322_BODY">On Mon, Mar 26, 2018 at 06:24:36PM +0300, Konstantin Belyavskiy wrote:<br>
> Please check most recent version.<br>
> branch: gh-3210-recover-missing-local-data-master-master<br><br>
Please don't send or submit a patch for 1.6 until we commit it to<br>
the trunk.</div></div></div></div></blockquote>Ok, let's finish with 1.9 first, then I will send you patch for 1.6.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15222291220000000322_BODY"><br><br>
I failed to find the patch in the mailing list. Pasting it here for<br>
review.<br><br>
> From 391448a496fd769ff6724cadab4d333d37a9088e Mon Sep 17 00:00:00 2001<br>
> From: Konstantin Belyavskiy <<a href="mailto:k.belyavskiy@tarantool.org">k.belyavskiy@tarantool.org</a>><br>
> Date: Tue, 13 Mar 2018 17:51:52 +0300<br>
> Subject: [PATCH] [replication] [recovery] recover missing data<br>
> <br>
> Recover missing local data from replica.<br>
> In case of sudden power-loss, if data was not written to WAL but<br>
> already sent to remote replica, local can't recover properly and<br>
> we have different datasets.<br>
> Fix it by using remote replica's data and LSN comparison.<br>
> Based on @GeorgyKirichenko proposal and @locker race free check.<br><br>
> Switch off replication/catch.test.lua<br><br>
Why? If the test is broken, please fix it. If you find the test<br>
pointless, delete it with a proper explanation.</div></div></div></div></blockquote>Actually this test was broken, since from description <br><p style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #4d2cdc; background-color: #fef49c;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #4d2cdc; background-color: #fef49c;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">-- Check that replica doesn't enter read-write mode before</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #4d2cdc; background-color: #fef49c;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #4d2cdc; background-color: #fef49c;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">-- catching up with the master: to check that we inject sleep into</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #4d2cdc; background-color: #fef49c;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #4d2cdc; background-color: #fef49c;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">-- the master relay_send function and attempt a data modifying</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #4d2cdc; background-color: #fef49c;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #4d2cdc; background-color: #fef49c;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">-- statement in replica while it's still fetching data from the</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #4d2cdc; background-color: #fef49c;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #4d2cdc; background-color: #fef49c;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">-- master.</span></p>it should be in read-only mode, but only now it works as expected. And I don't understand second part: <p style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #4d2cdc; background-color: #fef49c;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #4d2cdc; background-color: #fef49c;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">-- case #2: delete tuple by net.box</span></p>But my new test also checks this behaviour (remember you mention concurrency issue and suggest to<br>use read-only mode to fix it). So it's rather duplicated.<br>Ok, as for now, decided to update test and to enable it back.<br>Let's discuss about this test separately.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15222291220000000322_BODY"><br><br>
> <br>
> Closes #3210<br>
> <br>
> diff --git a/src/box/applier.cc b/src/box/applier.cc<br>
> index 6bfe5a99..5f0b3069 100644<br>
> --- a/src/box/applier.cc<br>
> +++ b/src/box/applier.cc<br>
> @@ -453,7 +453,8 @@ applier_subscribe(struct applier *applier)<br>
> }<br>
> <br>
> if (applier->state == APPLIER_SYNC &&<br>
> - applier->lag <= replication_sync_lag) {<br>
> + applier->lag <= replication_sync_lag &&<br>
> + vclock_compare(&applier->vclock, &replicaset.vclock) <= 0) {<br><br>
First, you use a wrong vclock - applier->vclock is the vclock at connect<br>
(the name is rather misleading though, true, we should probably rename<br>
it to remote_vclock_at_connect). You should use the vclock received in<br>
the SUBSCRIBE request.</div></div></div></div></blockquote>Initially I thought that applier->vclock is the same, but under certain condition it <br>could have different value, thanks for find it out. Also it makes new code easier<br>to understand.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15222291220000000322_BODY"><br><br>
Second, this new condition could use a comment.</div></div></div></div></blockquote>Add comment.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15222291220000000322_BODY"><br><br>
Third, this is a worthwhile change as is so I think it should be<br>
submitted in a separate patch.</div></div></div></div></blockquote>May be, so first submit new sync condition, then other parts?<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15222291220000000322_BODY"><br><br>
> /* Applier is synced, switch to "follow". */<br>
> applier_set_state(applier, APPLIER_FOLLOW);<br>
> }<br>
> diff --git a/src/box/relay.cc b/src/box/relay.cc<br>
> index 2bd05ad5..344a8e01 100644<br>
> --- a/src/box/relay.cc<br>
> +++ b/src/box/relay.cc<br>
> @@ -110,6 +110,11 @@ struct relay {<br>
> struct vclock recv_vclock;<br>
> /** Replicatoin slave version. */<br>
> uint32_t version_id;<br>
> + /**<br>
> + * Local master's LSN at the moment of subscribe, used to check<br>
> + * dataset on the other side and send missing data rows if any.<br>
> + */<br>
> + int64_t masters_lsn_at_subscribe;<br><br>
Why did you change the member name? I only asked to update the comment.<br>
'local_lsn_at_subscribe' is not perfect, but still a better name IMO.<br>
Actually, I'm thinking about storing a whole vclock here instead of just<br>
one LSN - that would help avoid confusion:<br><br>
/** Local vclock at the time of subscribe. */<br>
struct vclock local_vclock_at_subscribe;</div></div></div></div></blockquote>Done.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15222291220000000322_BODY"><br><br>
> <br>
> /** Relay endpoint */<br>
> struct cbus_endpoint endpoint;<br>
> diff --git a/src/box/wal.cc b/src/box/wal.cc<br>
> index 4576cfe0..4a43775d 100644<br>
> --- a/src/box/wal.cc<br>
> +++ b/src/box/wal.cc<br>
> @@ -768,8 +768,15 @@ wal_write(struct journal *journal, struct journal_entry *entry)<br>
> /*<br>
> * Find last row from local instance id<br>
> * and promote vclock.<br>
> + * In master-master configuration, during sudden<br>
> + * power-loss if data was not written to WAL but<br>
> + * already sent to others they will send back.<br>
> + * In this case we should update only local<br>
> + * vclock but not the replicaset one. Could be<br>
> + * checked by simple lsn comparison.<br><br>
I still don't understand this comment.<br><br>
> */<br>
> - if ((*last)->replica_id == instance_id) {<br>
> + if ((*last)->replica_id == instance_id &&<br>
> + replicaset.vclock.lsn[instance_id] < (*last)->lsn) {<br><br>
Use vclock_get() for this.<br><br>
Also, we agreed to move this to applier AFAIR.</div></div></div></div></blockquote>It's not so simple, attempt to check instance_id != row.replica_id to avoid double replicaset's<br>vclock promotion (just before <span data-mce-style="font-variant-ligatures: no-common-ligatures;" style="font-family: Menlo; font-size: 11px; background-color: rgb(254, 244, 156); font-variant-ligatures: no-common-ligatures;">xstream_write_xc</span>) fails, since it's not the only case of vclock<br>promotion and lead to wrong leaving read-only mode. I will try to explain it better.. but this<br>not works:<br><p style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; background-color: #fef49c;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; background-color: #fef49c;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;"><strong>--- a/src/box/applier.cc</strong></span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; background-color: #fef49c;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; background-color: #fef49c;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;"><strong>+++ b/src/box/applier.cc</strong></span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; background-color: #fef49c;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; background-color: #fef49c;"><span style="font-variant-ligatures: no-common-ligatures; color: #25b2bf;" data-mce-style="font-variant-ligatures: no-common-ligatures; color: #25b2bf;">@@ -503,8 +503,9 @@</span><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;"> applier_subscribe(struct applier *applier)</span></p><br><p style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #bd311b; background-color: #fef49c;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #bd311b; background-color: #fef49c;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">- vclock_follow(&replicaset.vclock, row.replica_id,</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #bd311b; background-color: #fef49c;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #bd311b; background-color: #fef49c;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">- row.lsn);</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #26b41b; background-color: #fef49c;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #26b41b; background-color: #fef49c;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">+</span><span style="font-variant-ligatures: no-common-ligatures; color: #000000;" data-mce-style="font-variant-ligatures: no-common-ligatures; color: #000000;"> </span><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">if (row.replica_id != instance_id)</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #26b41b; background-color: #fef49c;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; color: #26b41b; background-color: #fef49c;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">+</span><span style="font-variant-ligatures: no-common-ligatures; color: #000000;" data-mce-style="font-variant-ligatures: no-common-ligatures; color: #000000;"> </span><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;">vclock_follow(&replicaset.vclock, row.replica_id,</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; background-color: #fef49c;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; background-color: #fef49c;"><span style="font-variant-ligatures: no-common-ligatures; color: #26b41b;" data-mce-style="font-variant-ligatures: no-common-ligatures; color: #26b41b;">+</span><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;"> </span><span style="font-variant-ligatures: no-common-ligatures; color: #26b41b;" data-mce-style="font-variant-ligatures: no-common-ligatures; color: #26b41b;"> row.lsn);</span></p><p style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; background-color: #fef49c;" data-mce-style="margin-bottom: 0px; font-stretch: normal; font-size: 11px; line-height: normal; font-family: Menlo; background-color: #fef49c;"><span style="font-variant-ligatures: no-common-ligatures;" data-mce-style="font-variant-ligatures: no-common-ligatures;"> xstream_write_xc(applier->subscribe_stream, &row);</span></p><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15222291220000000322_BODY">> vclock_follow(&replicaset.vclock, instance_id,<br>
> (*last)->lsn);<br>
> break;<br>
> diff --git a/test/replication/on_replace.lua b/test/replication/on_replace.lua<br>
> index 7e49efe1..c5855892 100644<br>
> --- a/test/replication/on_replace.lua<br>
> +++ b/test/replication/on_replace.lua<br>
> @@ -22,13 +22,10 @@ box.cfg({<br>
> };<br>
> })<br>
> <br>
> -env = require('test_run')<br>
> -test_run = env.new()<br>
> -engine = test_run:get_cfg('engine')<br>
> -<br>
> box.once("bootstrap", function()<br>
> + local test_run = require('test_run').new()<br>
> box.schema.user.create(USER, { password = PASSWORD })<br>
> box.schema.user.grant(USER, 'replication')<br>
> - box.schema.space.create('test', {engine = engine})<br>
> + box.schema.space.create('test', {engine = test_run:get_cfg('engine')})<br>
> box.space.test:create_index('primary')<br>
> end)<br>
> diff --git a/test/replication/recover_missing.test.lua b/test/replication/recover_missing.test.lua<br>
> new file mode 100644<br>
> index 00000000..d5b0e0ad<br>
> --- /dev/null<br>
> +++ b/test/replication/recover_missing.test.lua<br>
> @@ -0,0 +1,42 @@<br>
> +env = require('test_run')<br>
> +test_run = env.new()<br>
> +<br>
> +SERVERS = { 'on_replace1', 'on_replace2' }<br><br>
Please use autobootstrap.lua - I want to see how it works with multiple<br>
masters.</div></div></div></div></blockquote>Ok, now 3 instances.<blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15222291220000000322_BODY">
> +-- Start servers<br>
> +test_run:create_cluster(SERVERS)<br>
> +-- Wait for full mesh<br>
> +test_run:wait_fullmesh(SERVERS)<br>
> +<br>
> +test_run:cmd("switch on_replace1")<br>
> +for i = 0, 9 do box.space.test:insert{i, 'test' .. i} end<br><br>
Nit: please start counting from 1, as this is common in Lua.</div></div></div></div></blockquote>I hope this is not necessary, just insert some random data to check it later )<blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15222291220000000322_BODY">
> +box.space.test:count()<br>
> +<br>
> +test_run:cmd('switch default')<br>
> +vclock1 = test_run:get_vclock('on_replace1')<br>
> +vclock2 = test_run:wait_cluster_vclock(SERVERS, vclock1)<br>
> +<br>
> +test_run:cmd("switch on_replace2")<br>
> +box.space.test:count()<br>
> +box.error.injection.set("ERRINJ_RELAY_TIMEOUT", 0.1)<br><br>
Decrease the timeout to speed up test execution time.</div></div></div></div></blockquote>Ok<blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15222291220000000322_BODY">
> +test_run:cmd("stop server on_replace1")<br>
> +fio = require('fio')<br>
> +-- This test checks ability to recover missing local data<br>
> +-- from remote replica. See #3210.<br>
> +-- Delete data on first master and test that after restart,<br>
> +-- due to difference in vclock it will be able to recover<br>
> +-- all missing data from replica.<br>
> +-- Also check that there is no concurrency, i.e. master is<br>
> +-- in 'read-only' mode unless it receives all data.<br>
> +fio.unlink(fio.pathjoin(fio.abspath("."), string.format('on_replace1/%020d.xlog', 8)))<br>
> +test_run:cmd("start server on_replace1")<br>
> +<br>
> +test_run:cmd("switch on_replace1")<br>
> +for i = 10, 19 do box.space.test:insert{i, 'test' .. i} end<br>
> +fiber = require('fiber')<br>
> +fiber.sleep(1)<br>
> +box.space.test:count()<br><br>
Use 'select' here to make sure the data received are correct.<br></div></div></div></div></blockquote>Ok<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15222291220000000322_BODY"><br>
> +<br>
> +-- Cleanup.<br>
> +test_run:cmd('switch default')<br>
> +test_run:drop_cluster(SERVERS)<br>
> +<br><br>
Nit: extra line at the end of the file.<br></div></div></div></div></blockquote>
<br>
<br>Best regards,<br>Konstantin Belyavskiy<br>k.belyavskiy@tarantool.org<br></BODY></HTML>