[Tarantool-patches] [PATCH v2 04/19] replication: make sync transactions wait quorum

[Vladislav Shpilevoy]

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.