Re: [Tarantool-patches] [PATCH 3/3] box: fix an assertion failure in box.ctl.promote()

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

23.05.2021 15:18, Vladislav Shpilevoy пишет:
> Hi! Thanks for the patch!
>
> I see some of CI jobs have failed the new test:
> https://github.com/tarantool/tarantool/runs/2620153809

Thanks for the review!
CI issues fixed in v2.

>
> See 4 comments below.
>
>> diff --git a/src/box/box.cc b/src/box/box.cc
>> index c10e0d8bf..1b1e7eec0 100644
>> --- a/src/box/box.cc
>> +++ b/src/box/box.cc
>> @@ -1442,17 +1442,22 @@ box_quorum_on_ack_f(struct trigger *trigger, void *event)
>>   }
>>   
>>   /**
>> - * Wait until at least @a quorum of nodes confirm @a target_lsn from the node
>> - * with id @a lead_id.
>> + * Wait until at least @a quorum of nodes confirm the last available synchronous
>> + * entry from the node with id @a lead_id.
>>    */
>>   static int
>> -box_wait_quorum(uint32_t lead_id, int64_t target_lsn, int quorum,
>> +box_wait_quorum(uint32_t lead_id, struct txn_limbo_entry **entry, int quorum,
>>   		double timeout)
> 1. Maybe try to leave this function not depending on the limbo
> and its entries? It was supposed to wait for replication of just
> LSN, not necessarily a synchronous transaction.

Ok. This made the code simpler IMO, but fine. I see your point.

>
>>   {
>>   	struct box_quorum_trigger t;
>>   	memset(&t, 0, sizeof(t));
>>   	vclock_create(&t.vclock);
>>   
>> +	*entry = txn_limbo_wait_lsn_assigned(&txn_limbo);
>> +	if (*entry == NULL)
>> +		return -1;
>> +	int64_t target_lsn = (*entry)->lsn;
>> +
>>   	/* Take this node into account immediately. */
>>   	int ack_count = vclock_get(box_vclock, lead_id) >= target_lsn;
>>   	replicaset_foreach(replica) {
>> @@ -1622,22 +1627,17 @@ box_promote(void)
>>   		}
>>   	}
>>   
>> -	/*
>> -	 * promote() is a no-op on the limbo owner, so all the rows
>> -	 * in the limbo must've come through the applier meaning they already
>> -	 * have an lsn assigned, even if their WAL write hasn't finished yet.
>> -	 */
>> -	wait_lsn = txn_limbo_last_synchro_entry(&txn_limbo)->lsn;
>> -	assert(wait_lsn > 0);
>> -
>> -	rc = box_wait_quorum(former_leader_id, wait_lsn, quorum,
>> +	struct txn_limbo_entry *last_entry;
>> +	rc = box_wait_quorum(former_leader_id,&last_entry, quorum,
> 2. Missing whitespace after the first argument.

Thanks

>
>>   			     replication_synchro_timeout);
>>   	if (rc == 0) {
>> +		wait_lsn = last_entry->lsn;
>>   		if (quorum < replication_synchro_quorum) {
>>   			diag_set(ClientError, ER_QUORUM_WAIT, quorum,
>>   				 "quorum was increased while waiting");
>>   			rc = -1;
>> -		} else if (wait_lsn < txn_limbo_last_synchro_entry(&txn_limbo)->lsn) {
>> +		} else if (last_entry !=
>> +			   txn_limbo_last_synchro_entry(&txn_limbo)) {
>>   			diag_set(ClientError, ER_QUORUM_WAIT, quorum,
>>   				 "new synchronous transactions appeared");
>>   			rc = -1;
> 3. Could all the 3 commits be replaced with calling wal_sync() in the
> beginning of the promote() if we see the last LSN is unknown? After
> wal_sync() several outcomes are possible:
>
> 	- All was rolled back, and the limbo is empty;
> 	- The last transaction is different after sync - it means
> 	  it was added during promote() which is an error like in
> 	  the code above;
> 	- The transaction in the end of the limbo is the same.
>
> In the last case you work like before - box_wait_quorum() with the
> known LSN. Will it work?

Yes, indeed. Thanks for pointing this out!
I took this approach in v2.

>
>> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
>> index f287369a2..406f2de89 100644
>> --- a/src/box/txn_limbo.c
>> +++ b/src/box/txn_limbo.c
>> @@ -69,6 +69,48 @@ txn_limbo_last_synchro_entry(struct txn_limbo *limbo)
>>   	return NULL;
>>   }
>>   
>> +static int
>> +txn_limbo_wait_lsn_assigned_f(struct trigger *trig, void *event)
>> +{
>> +	(void)event;
>> +	struct fiber *fiber = trig->data;
>> +	fiber_wakeup(fiber);
>> +	return 0;
>> +}
>> +
>> +struct txn_limbo_entry *
>> +txn_limbo_wait_lsn_assigned(struct txn_limbo *limbo)
>> +{
>> +	assert(!txn_limbo_is_empty(limbo));
>> +	struct txn_limbo_entry *entry = txn_limbo_last_synchro_entry(limbo);
>> +	if (entry->lsn >= 0)
>> +		return entry;
>> +
>> +	struct trigger write_trigger, rollback_trigger;
>> +	trigger_create(&write_trigger, txn_limbo_wait_lsn_assigned_f, fiber(),
>> +		       NULL);
>> +	trigger_create(&rollback_trigger, txn_limbo_wait_lsn_assigned_f,
>> +		       fiber(), NULL);
>> +	txn_on_wal_write(entry->txn, &write_trigger);
>> +	txn_on_rollback(entry->txn, &rollback_trigger);
>> +	do {
>> +		fiber_yield();
>> +		if (fiber_is_cancelled()) {
>> +			diag_set(FiberIsCancelled);
>> +			entry = NULL;
>> +			break;
>> +		}
>> +		if (entry->txn->signature < 0) {
>> +			diag_set(ClientError, ER_SYNC_ROLLBACK);
>> +			entry = NULL;
>> +			break;
>> +		}
>> +	} while (entry->lsn == -1);
>> +	trigger_clear(&write_trigger);
>> +	trigger_clear(&rollback_trigger);
> 4. Why do you need the LSN assigned in the on_wal_write trigger in
> the previous commit? I can't see where do you use it here.

I check for entry->lsn right after the waiter is woken.
It's woken from on_wal_write trigger, and the trigger also
wakes up the fiber which used to assign lsn.

The waiter is woken up first, so I had to assign lsn from
the trigger itself.

This is irrelevant now, though, since I've adopted your proposal
with wal_sync().


-- 
Serge Petrenko