<HTML><BODY>Hi! Thanks for your detailed answer!<br>I pushed v2 regarding all the comments.<br><br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Четверг, 13 февраля 2020, 9:47 +03:00 от Konstantin Osipov <kostja.osipov@gmail.com>:<br><br><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15815764551538883736_BODY">* sergepetrenko <<a href="mailto:sergepetrenko@tarantool.org">sergepetrenko@tarantool.org</a>> [20/02/13 09:34]:<br>
> Fix replicaset.applier.vclock initialization issues: it wasn't<br>
> initialized at all previously.<br><br>
In the next line you say that you remove the initialization. What<br>
do you mean here?</div></div></div></div></blockquote><br>I changed both the commit and the commit message.<br><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_15815764551538883736_BODY"><br><br>
> Moreover, there is no valid point in code<br>
> to initialize it, since it may get stale right away if new entries are<br>
> written to WAL.<br><br>
Well, it reflects the state of the wal *as seen by* the set of<br>
appliers. This is stated in the comment. So it doesn't have to<br>
reflect local changes. </div></div></div></div></blockquote><br>I see.<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_15815764551538883736_BODY"><br><br>
> So, check for both applier and replicaset vclocks.<br>
> The greater one protects the instance from applying the rows it has<br>
> already applied or has already scheduled to write.<br>
> Also remove an unnecessary aplier vclock initialization from<br>
> replication_init().<br><br>
First of all, the race you describe applies to<br>
local changes only. Yet you add the check for all replica ids. <br>
This further obliterates this piece of code.</div></div></div></div></blockquote><br>Ok, fixed.<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_15815764551538883736_BODY"><br><br>
Second, the core of the issue is a "hole" in vclock protection<br>
enforced by latch_lock/latch_unlock. Basically the assumption that<br>
latch_lock/latch_unlock has is that while a latch is locked, no<br>
source can apply a transaction under this replica id. This, is<br>
violated by the local WAL.<br><br>
We used to skip all changes by local vclock id before in applier.<br><br>
Later it was changed to be able to get-your-own logs on recovery,<br>
e.g. if some replica has them , and the local node lost a piece of<br>
wal.<br><br>
It will take me a while to find this commit and ticket, but this<br>
is the commit and ticket which introduced the regression.<br><br>
The proper fix is to only apply local changes received from<br>
remotes in orphan mode, and begin skipping them when entering<br>
read-write mode.</div></div></div></div></blockquote><br>Thanks for the clarification.<br><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_15815764551538883736_BODY"><br><br>
> Closes #4739<br>
> ---<br>
> src/box/applier.cc | 14 ++++++++++++--<br>
> src/box/replication.cc | 1 -<br>
> 2 files changed, 12 insertions(+), 3 deletions(-)<br>
> <br>
> diff --git a/src/box/applier.cc b/src/box/applier.cc<br>
> index ae3d281a5..acb26b7e2 100644<br>
> --- a/src/box/applier.cc<br>
> +++ b/src/box/applier.cc<br>
> @@ -731,8 +731,18 @@ applier_apply_tx(struct stailq *rows)<br>
> struct latch *latch = (replica ? &replica->order_latch :<br>
> &replicaset.applier.order_latch);<br>
> latch_lock(latch);<br>
> - if (vclock_get(&replicaset.applier.vclock,<br>
> - first_row->replica_id) >= first_row->lsn) {<br>
> + /*<br>
> + * We cannot tell which vclock is greater. There is no<br>
> + * proper place to initialize applier vclock, since it<br>
> + * may get stale right away if we write something to WAL<br>
> + * and it gets replicated and then arrives back from the<br>
> + * replica. So check against both vclocks. Replicaset<br>
> + * vclock will guard us from corner cases like the one<br>
> + * above.<br>
> + */<br>
> + if (MAX(vclock_get(&replicaset.applier.vclock, first_row->replica_id),<br>
> + vclock_get(&replicaset.vclock, first_row->replica_id)) >=<br>
> + first_row->lsn) {<br>
> latch_unlock(latch);<br>
> return 0;<br>
> }<br>
> diff --git a/src/box/replication.cc b/src/box/replication.cc<br>
> index e7bfa22ab..7b04573a4 100644<br>
> --- a/src/box/replication.cc<br>
> +++ b/src/box/replication.cc<br>
> @@ -93,7 +93,6 @@ replication_init(void)<br>
> latch_create(&replicaset.applier.order_latch);<br>
> <br>
> vclock_create(&replicaset.applier.vclock);<br>
> - vclock_copy(&replicaset.applier.vclock, &replicaset.vclock);<br>
> rlist_create(&replicaset.applier.on_rollback);<br>
> rlist_create(&replicaset.applier.on_commit);<br>
> <br>
> -- <br>
> 2.20.1 (Apple Git-117)<br><br>
-- <br>
Konstantin Osipov, Moscow, Russia<br></div></div></div></div></blockquote>
<br>
<br>-- <br>Sergey Petrenko<br></BODY></HTML>