Re: [Tarantool-patches] [PATCH 1/3] vinyl: handle multi-statement recovery txns

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

02.04.2021 01:23, Vladislav Shpilevoy пишет:
> During recovery and xlog replay vinyl skips the statements already
> stored in runs. Indeed, their re-insertion into the mems would
> lead to their second dump otherwise.
>
> But that results into an issue that the recovery transactions in
> vinyl don't have a write set - their tx->log is empty. On the
> other hand they still are added to the write set (xm->writers).
> Probably so as not to have too many checks "skip if in recovery"
> all over the code.
>
> It works fine with single-statement transactions, but would break
> on multi-statement transactions. Because the decision whether
> need to add to the write set was done based on the tx's log
> emptiness. It is always empty, and so the transaction could be
> added to the write set twice and corrupt its list-link member.
>
> The patch makes the decision about being added to the write set
> based on emptiness of the list-link member instead of the log so
> it works fine both during recovery and normal operation.
>
> Needed for #5874

Hi! Thanks for the patch!
Looks fine at first glance.

> ---
>   src/box/vy_tx.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
> index ff63cd7a1..cd210beb0 100644
> --- a/src/box/vy_tx.c
> +++ b/src/box/vy_tx.c
> @@ -899,8 +899,15 @@ vy_tx_begin_statement(struct vy_tx *tx, struct space *space, void **savepoint)
>   	}
>   	assert(tx->state == VINYL_TX_READY);
>   	tx->last_stmt_space = space;
> -	if (stailq_empty(&tx->log))
> +	/*
> +	 * When want to add to the writer list, can't rely on the log emptiness.
> +	 * During recovery it is empty always for the data stored both in runs
> +	 * and xlogs. Must check the list member explicitly.
> +	 */
> +	if (rlist_empty(&tx->in_writers)) {
> +		assert(stailq_empty(&tx->log));
>   		rlist_add_entry(&tx->xm->writers, tx, in_writers);
> +	}
>   	*savepoint = stailq_last(&tx->log);
>   	return 0;
>   }

-- 
Serge Petrenko