<HTML><BODY>Vladimir,<br><div class="js-helper js-readmsg-msg"><div><div id="style_15220650090000000581_BODY"><div class="class_1522090696"><br>Please check most recent version.<br><div class="js-helper_mailru_css_attribute_postfix js-readmsg-msg_mailru_css_attribute_postfix"><div><div id="style_15220593980000000892_BODY_mailru_css_attribute_postfix"><div class="class_1522073966_mailru_css_attribute_postfix">branch: gh-3210-recover-missing-local-data-master-master<br><br>Please note, that only branch for Tarantool 1.9 is discussed here, so please check another letter for v.1.6 branch.<br><br> <blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;">
Среда, 21 марта 2018, 19:34 +03:00 от Vladimir Davydov <<a href="//e.mail.ru/compose/?mailto=mailto%3avdavydov.dev@gmail.com" target="_blank" rel=" noopener noreferrer">vdavydov.dev@gmail.com</a>>:<br><br><div id=""><div class="js-helper_mailru_css_attribute_postfix_mailru_css_attribute_postfix js-readmsg-msg_mailru_css_attribute_postfix_mailru_css_attribute_postfix"><div><div id="style_15216500880000000666_BODY_mailru_css_attribute_postfix_mailru_css_attribute_postfix">On Wed, Mar 21, 2018 at 04:47:46PM +0300, Konstantin Belyavskiy wrote:<br>
> Please take a look at the new version.<br><br>
I don't see the new patch in the mailing list nor in this email so I<br>
have to check out the branch, which is inconvenient for me. Please<br>
always make sure that your patches can be found in the mailing list.<br></div></div></div></div></blockquote> Separated<br><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div id=""><div class="js-helper_mailru_css_attribute_postfix_mailru_css_attribute_postfix js-readmsg-msg_mailru_css_attribute_postfix_mailru_css_attribute_postfix"><div><div id="style_15216500880000000666_BODY_mailru_css_attribute_postfix_mailru_css_attribute_postfix"><br>
> <br>
> >Четверг, 15 марта 2018, 17:10 +03:00 от Konstantin Osipov <<a href="//e.mail.ru/compose/?mailto=mailto%3akostja@tarantool.org" target="_blank" rel=" noopener noreferrer">kostja@tarantool.org</a>>:<br>
> ><br>
> >* Konstantin Belyavskiy < <a href="//e.mail.ru/compose/?mailto=mailto%3ak.belyavskiy@tarantool.org" target="_blank" rel=" noopener noreferrer">k.belyavskiy@tarantool.org</a> > [18/03/15 11:39]:<br>
> ><br>
> >The patch has to go to 1.6, since that's where the customer<br>
> >complained about it.<br>
> ><br>
> >Is it feasible to backport? <br>
> for 1.9 branch: gh-3210-recover-missing-local-data-master-master<br>
> for 1.6 branch: gh-3210-recover-missing-local-data-master-master-16 *<br><br>
$ git log --oneline origin/gh-3210-recover-missing-local-data-master-master-16 | head -n3<br>
2aa60f15 remove test<br>
4e2fa3b9 [replication] [recovery] recover missing data<br>
b80868f2 [replication] [recovery] recover missing data<br><br>
Squash them, please.<br><br>
Pasting the patch from the branch here for review:<br><br>
> From 20e1f76c9ec829f48a0899fa71f805a01ef5be42 Mon Sep 17 00:00:00 2001<br>
> From: Konstantin Belyavskiy <<a href="//e.mail.ru/compose/?mailto=mailto%3ak.belyavskiy@tarantool.org" target="_blank" rel=" noopener noreferrer">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<br>
> <br>
> Closes #3210<br>
> <br>
> diff --git a/src/box/relay.cc b/src/box/relay.cc<br>
> index 2bd05ad5..86c18ecb 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>
> + * LSN at the moment of subscribe, used to check dataset<br>
> + * on the other side and send missing data rows if any.<br>
> + */<br>
> + int64_t local_lsn_before_subscribe;<br><br>
It isn't clear from the comment what LSN it is, namely that this is LSN<br>
of the replica as the master sees it.</div></div></div></div></blockquote>Updated<br><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div id=""><div class="js-helper_mailru_css_attribute_postfix_mailru_css_attribute_postfix js-readmsg-msg_mailru_css_attribute_postfix_mailru_css_attribute_postfix"><div><div id="style_15216500880000000666_BODY_mailru_css_attribute_postfix_mailru_css_attribute_postfix"><br><br>
> <br>
> /** Relay endpoint */<br>
> struct cbus_endpoint endpoint;<br>
> @@ -541,6 +546,7 @@ relay_subscribe(int fd, uint64_t sync, struct replica *replica,<br>
> relay.version_id = replica_version_id;<br>
> relay.replica = replica;<br>
> replica_set_relay(replica, &relay);<br>
> + relay.local_lsn_before_subscribe = vclock_get(&replicaset.vclock, replica->id);<br>
> <br>
> int rc = cord_costart(&relay.cord, tt_sprintf("relay_%p", &relay),<br>
> relay_subscribe_f, &relay);<br>
> @@ -583,10 +589,15 @@ relay_send_row(struct xstream *stream, struct xrow_header *packet)<br>
> /*<br>
> * We're feeding a WAL, thus responding to SUBSCRIBE request.<br>
> * In that case, only send a row if it is not from the same replica<br>
> - * (i.e. don't send replica's own rows back).<br>
> + * (i.e. don't send replica's own rows back) or if this row is<br>
> + * missing on the other side (i.e. in case of sudden power-loss,<br>
> + * data was not written to WAL, so remote replica can't recover<br>
> + * it). In this case packet's LSN is lower or equal then local<br>
> + * replica's LSN at the moment it has issued 'SUBSCRIBE' request.<br>
> */<br><br>
To be honest, I don't understand why we should fix this at all. If WAL<br>
is written without syncing, there's no guarantee that there will be no<br>
data loss in case of power outage.<br><br>
Anyway, the fix seems to be inherently racy, because the master that<br>
experienced a data loss might start writing new records to WAL right<br>
after local recovery, even before it receives its own lost records from<br>
the replica. What will happen then?<br></div></div></div></div></blockquote>Fixed with @locker proposal based on 'read-only' mode.<br><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div id=""><div class="js-helper_mailru_css_attribute_postfix_mailru_css_attribute_postfix js-readmsg-msg_mailru_css_attribute_postfix_mailru_css_attribute_postfix"><div><div id="style_15216500880000000666_BODY_mailru_css_attribute_postfix_mailru_css_attribute_postfix"><br>
> if (relay->replica == NULL ||<br>
> - packet->replica_id != relay->replica->id) {<br>
> + packet->replica_id != relay->replica->id ||<br>
> + packet->lsn <= relay->local_lsn_before_subscribe) {<br>
> relay_send(relay, packet);<br>
> }<br>
> }<br>
> diff --git a/src/box/wal.cc b/src/box/wal.cc<br>
> index 4576cfe0..c321b2de 100644<br>
> --- a/src/box/wal.cc<br>
> +++ b/src/box/wal.cc<br>
> @@ -768,8 +768,16 @@ 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'm afraid I don't understand the comment. Is this a counter-measure<br>
against the race condition I wrote about above?</div></div></div></div></blockquote>I'm trying to explain here, that in case we got master's own row, but it was missed, we<br>should not update replicaset vclock, but only local vclock.<br>So we should skip vclock_follow here.<br><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div id=""><div class="js-helper_mailru_css_attribute_postfix_mailru_css_attribute_postfix js-readmsg-msg_mailru_css_attribute_postfix_mailru_css_attribute_postfix"><div><div id="style_15216500880000000666_BODY_mailru_css_attribute_postfix_mailru_css_attribute_postfix"><br><br>
> + * See #3210 for details.<br><br>
Please don't refer to GitHub tickets in the code - comments alone should<br>
be enough.</div></div></div></div></blockquote>Fixed.<blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div id=""><div class="js-helper_mailru_css_attribute_postfix_mailru_css_attribute_postfix js-readmsg-msg_mailru_css_attribute_postfix_mailru_css_attribute_postfix"><div><div id="style_15216500880000000666_BODY_mailru_css_attribute_postfix_mailru_css_attribute_postfix"><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>
There's vclock_get() helper for this.<br><br>
> vclock_follow(&replicaset.vclock, instance_id,<br>
> (*last)->lsn);<br>
> break;<br><br>
> diff --git a/test/replication/master1.lua b/test/replication/master1.lua<br>
> new file mode 120000<br>
> diff --git a/test/replication/master2.lua b/test/replication/master2.lua<br>
> new file mode 120000<br>
> diff --git a/test/replication/master_master.lua b/test/replication/master_master.lua<br>
> new file mode 100644<br><br>
Do you really need all these new files. Can't you reuse<br>
autobootstrap.lua?</div></div></div></div></blockquote>Fixed, based on 'on_replace.lua' since it's closer than autobootstrap.lua (2 instance vs. 3)<br><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div id=""><div class="js-helper_mailru_css_attribute_postfix_mailru_css_attribute_postfix js-readmsg-msg_mailru_css_attribute_postfix_mailru_css_attribute_postfix"><div><div id="style_15216500880000000666_BODY_mailru_css_attribute_postfix_mailru_css_attribute_postfix"><br><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..2f36fc32<br>
> --- /dev/null<br>
> +++ b/test/replication/recover_missing.test.lua<br>
> @@ -0,0 +1,30 @@<br><br>
Please add a comment describing what you're testing here and the GitHub<br>
ticket number, as we usually do.<br><br>
> +env = require('test_run')<br>
> +test_run = env.new()<br>
> +<br>
> +SERVERS = { 'master1', 'master2' }<br>
> +-- 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 master1")<br>
> +box.space._schema:insert({'1'})<br>
> +box.space._schema:select('1')<br><br>
Please don't use system spaces for tests.</div></div></div></div></blockquote>Fixed.<br><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div id=""><div class="js-helper_mailru_css_attribute_postfix_mailru_css_attribute_postfix js-readmsg-msg_mailru_css_attribute_postfix_mailru_css_attribute_postfix"><div><div id="style_15216500880000000666_BODY_mailru_css_attribute_postfix_mailru_css_attribute_postfix"><br><br>
> +<br>
> +fiber = require('fiber')<br>
> +fiber.sleep(0.1)<br><br>
Waiting for a change to be relayed like this is racy. Either use<br>
wait_cluster_vclock() helper or loop on the replica until it sees the<br>
change.</div></div></div></div></blockquote>Fixed.<br><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><div id=""><div class="js-helper_mailru_css_attribute_postfix_mailru_css_attribute_postfix js-readmsg-msg_mailru_css_attribute_postfix_mailru_css_attribute_postfix"><div><div id="style_15216500880000000666_BODY_mailru_css_attribute_postfix_mailru_css_attribute_postfix"><br><br>
> +<br>
> +test_run:cmd("switch master2")<br>
> +box.space._schema:select('1')<br>
> +test_run:cmd("stop server master1")<br>
> +fio = require('fio')<br>
> +fio.unlink(fio.pathjoin(fio.abspath("."), string.format('master1/%020d.xlog', 8)))<br>
> +test_run:cmd("start server master1")<br>
> +<br>
> +test_run:cmd("switch master1")<br>
> +box.space._schema:select('1')<br>
> +<br>
> +test_run:cmd("switch default")<br>
> +test_run:cmd("stop server master1")<br>
> +test_run:cmd("stop server master2")<br><br>
Use drop_cluster() for cleanup.<br></div></div></div></div></blockquote>
Fixed.<br>
<br>Best regards,<br>Konstantin Belyavskiy<br><a href="//e.mail.ru/compose/?mailto=mailto%3ak.belyavskiy@tarantool.org" target="_blank" rel=" noopener noreferrer">k.belyavskiy@tarantool.org</a><br>
</div></div>
</div>
</div>
<br><hr>
<br>Best regards,<br>Konstantin Belyavskiy<br><a href="mailto:k.belyavskiy@tarantool.org">k.belyavskiy@tarantool.org</a><br><style></style>
</div></div>
<base target="_self" href="https://e.mail.ru/">
</div>
</div>
<br><hr>
<br>Best regards,<br>Konstantin Belyavskiy<br>k.belyavskiy@tarantool.org<br><style type="text/css"></style></BODY></HTML>