From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id 7FB67469719
 for <tarantool-patches@dev.tarantool.org>;
 Sun, 16 Feb 2020 19:15:49 +0300 (MSK)
References: <cover.1581630406.git.sergepetrenko@tarantool.org>
 <97f37279955ebc29e899164cc1364e6a0aea0f9b.1581630406.git.sergepetrenko@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <c19b8c03-d9b1-33eb-70a0-0acc1b59e638@tarantool.org>
Date: Sun, 16 Feb 2020 17:15:46 +0100
MIME-Version: 1.0
In-Reply-To: <97f37279955ebc29e899164cc1364e6a0aea0f9b.1581630406.git.sergepetrenko@tarantool.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH v2 3/4] wal: wart when trying to
 write a record with a broken lsn
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: sergepetrenko <sergepetrenko@tarantool.org>, alexander.turenko@tarantool.org, kostja.osipov@gmail.com
Cc: tarantool-patches@dev.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.