From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D9DDB4429E1 for ; Sat, 4 Jul 2020 00:16:38 +0300 (MSK) From: Vladislav Shpilevoy References: <38933b569988f89ad124e71e49b460698db5e4a0.1593472477.git.v.shpilevoy@tarantool.org> <45912c93-76ed-3913-cfb3-057b948556b4@tarantool.org> Message-ID: <37ade6e8-3ad9-3656-3793-d05d664c317a@tarantool.org> Date: Fri, 3 Jul 2020 23:16:36 +0200 MIME-Version: 1.0 In-Reply-To: <45912c93-76ed-3913-cfb3-057b948556b4@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH v2 04/19] replication: make sync transactions wait quorum List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Serge Petrenko , tarantool-patches@dev.tarantool.org Thanks for the review! (The email was intended to be sent yesterday, but my email client somehow screw it.) >> diff --git a/src/box/txn.c b/src/box/txn.c >> index edc1f5180..6cfa98212 100644 >> --- a/src/box/txn.c >> +++ b/src/box/txn.c >> @@ -658,7 +701,11 @@ txn_commit(struct txn *txn) >>           diag_log(); >>           return -1; >>       } >> - >> +    if (is_sync) { >> +        txn_limbo_assign_lsn(&txn_limbo, limbo_entry, >> +                     req->rows[req->n_rows - 1]->lsn); > > This assumes that the last tx row is a global one. This'll be true once > #4928 [1] is fixed. However, the fix is a crutch, either appending a dummy NOP > statement at the end of tx, or reordering the rows so that the last one is > global. If we remove the crutch someday, we'll also break LSN assignment here. > Maybe there's a better way to assign LSN to a synchronous tx? > > Another point. Once async transactions are also added to limbo, we'll > have fully local transactions in limbo. Local transactions have a separate > LSN counter, so we probably have to assign them the same LSN as the last > synchronous transaction waiting in limbo. Looks like this'll work. Good point. Use of the last LSN won't work though, because the last sync transaction in the limbo still may have not assigned LSN, i.e. -1. So we can't take it. Because it does not exist yet. I thought about making txn_limbo_assign_lsn() do that for all next async transactions, but looks like a crutch. Instead, I make the async transaction appearance more explicit - their LSNs are always -1 in the limbo and are never changed. See a new email thread with the solution and some tests. > [1] https://github.com/tarantool/tarantool/issues/4928 I added a test for sync transactions having a local row last. It is disabled until 4928 is fixed. And will broke, when we will revert the crutch in favor of whatever the other solution will be. So we will notice.