Re: [Tarantool-patches] [PATCH v20 2/3] qsync: order access to the limbo terms

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

06.10.2021 23:15, Cyrill Gorcunov пишет:
> Limbo terms tracking is shared between appliers and when
> one of appliers is waiting for write to complete inside
> journal_write() routine, an other may need to access read
> term value to figure out if promote request is valid to
> apply. Due to cooperative multitasking access to the terms
> is not consistent so we need to be sure that other fibers
> read up to date terms (ie written to the WAL).
>
>

Thanks for working on this!

LGTM with one minor comment.

...

> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index 70447caaf..301fc1a8f 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c

...

> @@ -724,11 +738,14 @@ txn_limbo_wait_empty(struct txn_limbo *limbo, double timeout)
>   }
>   
>   void
> -txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req)
> +txn_limbo_process_core(struct txn_limbo *limbo,
> +		       const struct synchro_request *req)
>   {
> +	assert(latch_is_locked(&limbo->promote_latch));
> +
>   	uint64_t term = req->term;
>   	uint32_t origin = req->origin_id;
> -	if (txn_limbo_replica_term(limbo, origin) < term) {
> +	if (vclock_get(&limbo->promote_term_map, origin) < (int64_t)term) {
>   		vclock_follow(&limbo->promote_term_map, origin, term);
>   		if (term > limbo->promote_greatest_term)
>   			limbo->promote_greatest_term = term;
> @@ -786,11 +803,33 @@ txn_limbo_process(struct txn_limbo *limbo, const struct synchro_request *req)
>   	return;
>   }
>   
> +void
> +txn_limbo_process(struct txn_limbo *limbo,
> +		  const struct synchro_request *req)
> +{
> +	txn_limbo_process_begin(limbo);
> +	txn_limbo_process_core(limbo, req);
> +	txn_limbo_process_commit(limbo);
> +}
> +
>   void
>   txn_limbo_on_parameters_change(struct txn_limbo *limbo)
>   {
>   	if (rlist_empty(&limbo->queue))
>   		return;
> +	/*
> +	 * In case if we're not current leader (ie not owning the
> +	 * limbo) then we should not confirm anything, otherwise
> +	 * we could reduce quorum number and start writing CONFIRM
> +	 * while leader node carries own maybe bigger quorum value.
> +	 */
> +	if (!txn_limbo_is_owner(limbo, instance_id)) {
> +		say_warn("qsync: trying to change replication_synchro_quorum "
> +			 "on node %u while owner is %u, ignore.",
> +			 instance_id, limbo->owner_id);
> +		return;
> +	}

Hm, I don't think we need a warning here. It'd be printed every time quorum
is updated due to formula recalculations.


> +
>   	struct txn_limbo_entry *e;
>   	int64_t confirm_lsn = -1;
>   	rlist_foreach_entry(e, &limbo->queue, in_queue) {
>
...

-- 
Serge Petrenko