From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <sergos@tarantool.org>
Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100])
 (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 B700446970F
 for <tarantool-patches@dev.tarantool.org>;
 Wed, 27 Nov 2019 16:40:57 +0300 (MSK)
Date: Wed, 27 Nov 2019 16:40:56 +0300
From: Sergey Ostanevich <sergos@tarantool.org>
Message-ID: <20191127134056.GD1718@tarantool.org>
References: <cover.1574390065.git.i.kosarev@tarantool.org>
 <1673b241d9ca3f1f3396c158d298845907fb5392.1574390065.git.i.kosarev@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <1673b241d9ca3f1f3396c158d298845907fb5392.1574390065.git.i.kosarev@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v5 4/8] refactoring: set diagnostics
 if sequence_by_id fails
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: Ilya Kosarev <i.kosarev@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

Hi!

LGTM. 

Please, create a follow-up tracker to review all sequence_by_id() uses:
for example, in on_drop_sequence_data_rollback() this can lead to a
coredump inside the sequence_set() in release build. 

Regards,
Sergos

On 22 Nov 05:46, Ilya Kosarev wrote:
> In refactoring: use non _xc version of functions in triggers
> (b75d5f85338158eb1734ca8832d3c4b8ac0b4086) sequence_cache_find was
> replaced by sequence_by_id. It led to the loss of diagnostics in case
> of sequence_by_id failure. Now it is fixed.
> 
> Part of #4247
> ---
>  src/box/alter.cc | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index aaaf53493..2628e1401 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -4365,8 +4365,10 @@ on_replace_dd_sequence_data(struct trigger * /* trigger */, void *event)
>  			    &id) != 0)
>  		return -1;
>  	struct sequence *seq = sequence_by_id(id);
> -	if (seq == NULL)
> +	if (seq == NULL) {
> +		diag_set(ClientError, ER_NO_SUCH_SEQUENCE, int2str(id));
>  		return -1;
> +	}
>  	if (new_tuple != NULL) {			/* INSERT, UPDATE */
>  		int64_t value;
>  		if (tuple_field_i64(new_tuple, BOX_SEQUENCE_DATA_FIELD_VALUE,
> @@ -4520,8 +4522,10 @@ on_replace_dd_space_sequence(struct trigger * /* trigger */, void *event)
>  	if (space == NULL)
>  		return -1;
>  	struct sequence *seq = sequence_by_id(sequence_id);
> -	if (seq == NULL)
> +	if (seq == NULL) {
> +		diag_set(ClientError, ER_NO_SUCH_SEQUENCE, int2str(sequence_id));
>  		return -1;
> +	}
>  
>  	enum priv_type priv_type = stmt->new_tuple ? PRIV_C : PRIV_D;
>  	if (stmt->new_tuple && stmt->old_tuple)
> -- 
> 2.17.1
>