Re: [Tarantool-patches] [PATCH v2 19/19] replication: block async transactions when not empty limbo

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the review!

> I would like to see it merged with 03 and 04 of this series. It's no
> good to have a self-rewrite in the same patchset, isn't it?

It is not good, when it is done in a weird way which could be easily
avoided. In this patchset this particular commit can't be just merged
into patches 3 and 4. It affects more patches.

Nonetheless I managed to split it in parts. The code changes were
merged into patches 3, 4, 7, 8, 10. The original commit is kept where
it is, and now contains only the test. The test depends on ROLLBACK
and CONFIRM existence, therefore can't be merged into patches 3 and 4
(where most of the limbo appears).

> Plus some nits. 
> 
> Sergos
> 
> On 30 Jun 01:15, Vladislav Shpilevoy wrote:
>> When there is a not committed synchronous transaction, any attempt
>              are transactions it the limbo

What is wrong? If there is a not committed sync transaction, it means
it is in the limbo.

>> to commit a next transaction should be suspended, even if it is an
>> async transaction.
>>
>> This restriction comes from the theoretically possible dependency
>> of what is written in the async transactions on what was written
>> in the previous sync transactions.
>>
>> For that there is a new txn flag - TXN_WAIT_SYNC. Previously the
>> only synchro replication flag was TXN_WAIT_ACK. And now a
>> transaction can be sync, but not wait for ACKs.
> 
> This one took some time for me to grasp - you say an async transaction
> can be sync, which put me to a stop. I believe if join this with ACK
> description it will become clear.

This part of the text is removed. As well as the patch 3 was squashed
into patch 4.

>> In particular, if a transaction:
>>
>> - Is synchronous, the it has TXN_WAIT_SYNC (it is sync), and
>                     ^^^^^^
>>   TXN_WAIT_ACK (need to collect ACKs, or get a CONFIRM);
>>
>> - Is asynchronous, and the limbo was empty and the moment of
>>   commit, the it does not have any of these flags and committed
>>   like earlier;
>>
>> - Is asynchronous, and the limbo was not empty and the moment of
>>   commit. Then it will have only TXN_WAIT_SYNC. So it will be
>>   finished right after all the previous sync transactions are
>>   done. Note: *without waiting for ACKs* - the transaction is
>>   still asynchronous in a sense that it is don't need to wait for
>>   quorum replication.
> 
> So, there should be no TXN_WAIT_ACK set without the TXN_WAIT_SYNC. Is
> there an assertion for this?

Yes, there is plenty of assertions on that.

>> Follow-up #4845
>> ---
>> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
>> index fbe4dcecf..bfb404e8e 100644
>> --- a/src/box/txn_limbo.c
>> +++ b/src/box/txn_limbo.c
>> @@ -238,11 +247,21 @@ txn_limbo_read_confirm(struct txn_limbo *limbo, int64_t lsn)
>>  	assert(limbo->instance_id != REPLICA_ID_NIL);
>>  	struct txn_limbo_entry *e, *tmp;
>>  	rlist_foreach_entry_safe(e, &limbo->queue, in_queue, tmp) {
>> -		if (e->lsn > lsn)
>> +		/*
>> +		 * Confirm a transaction if
>> +		 * - it is a sync transaction covered by the
>> +		 *   confirmation LSN;
>> +		 * - it is an async transaction, and it is the
>> +		 *   last in the queue. So it does not depend on
>> +		 *   a not finished sync transaction anymore and
>                        ------------  waiting for ACK^------- 

Yes, this is the same.