Re: [Tarantool-patches] [PATCH v2 3/4] wal: wart when trying to write a record with a broken lsn

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

Hi! Thanks for the patch! I will review other commits when
Kostja is fine with them.

Since he finished with this one, here is my nit: lets keep
assertion for the debug build. If not this assertion, we
probably wouldn't notice this bug, and may miss future bugs
without it. During running the tests we won't notice a
warning.

Also we probably should not call vclock_follow() at all, if
lsn is broken. Just keep it as it. It does not look right to
decrease it. And make it panic() in vclock_follow() to catch
other bugs related to it.

On 13/02/2020 22:52, sergepetrenko wrote:
> From: Serge Petrenko <sergepetrenko@tarantool.org>
> 
> There is an assertion in vclock_follow `lsn > prev_lsn`, which doesn't
> fire in release builds, of course. Let's at least warn the user on an
> attemt to write a record with a duplicate or otherwise broken lsn.
> 
> Follow-up #4739
> ---
>  src/box/wal.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/src/box/wal.c b/src/box/wal.c
> index 0ae66ff32..f8ee2b7d8 100644
> --- a/src/box/wal.c
> +++ b/src/box/wal.c
> @@ -951,9 +951,18 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base,
>  			(*row)->tsn = tsn;
>  			(*row)->is_commit = row == end - 1;
>  		} else {
> -			vclock_follow(vclock_diff, (*row)->replica_id,
> -				      (*row)->lsn - vclock_get(base,
> -							       (*row)->replica_id));
> +			int64_t diff = (*row)->lsn - vclock_get(base, (*row)->replica_id);
> +			if (diff <= vclock_get(vclock_diff,
> +					       (*row)->replica_id)) {
> +				say_crit("Attempt to write a broken LSN to WAL:"
> +					 " replica id: %d, committed lsn: %d,"
> +					 " new lsn %d", (*row)->replica_id,
> +					 vclock_get(base, (*row)->replica_id) +
> +					 vclock_get(vclock_diff,
> +						    (*row)->replica_id),
> +						    (*row)->lsn);
> +			}
> +			vclock_follow(vclock_diff, (*row)->replica_id, diff);

On the summary, lets call follow() in 'else' branch, and add unreachable()
after crit log.