From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <sergos@tarantool.org>
Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37])
 (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 898A946970E
 for <tarantool-patches@dev.tarantool.org>;
 Mon, 16 Dec 2019 14:55:19 +0300 (MSK)
Date: Mon, 16 Dec 2019 14:55:18 +0300
From: Sergey Ostanevich <sergos@tarantool.org>
Message-ID: <20191216115518.GA26828@tarantool.org>
References: <cover.1569253692.git.i.kosarev@tarantool.org>
 <20191112105951.GC10433@tarantool.org>
 <1573572765.685045368@f111.i.mail.ru>
 <1574176592.966016381@f196.i.mail.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1574176592.966016381@f196.i.mail.ru>
Subject: Re: [Tarantool-patches] [tarantool-patches] Re[2]:
 [tarantool-patches] Re: [PATCH v4 12/20] refactoring: remove exceptions
 from space_def_new_from_tuple
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: tarantool-patches@freelists.org
Cc: tarantool-patches <tarantool-patches@dev.tarantool.org>

Hi!

Thanks, Follow-up is fine - the patch is LGTM.

Sergos

On 19 Nov 18:16, Ilya Kosarev wrote:
> 
> Hi!
> After brief discussion with Nikita we found out that
> sqlRunParser should set diagnostics for all error cases,
> though it doesn't. Thus i submitted an issue:
> https://github.com/tarantool/tarantool/issues/4633
> 
> Sincerely,
> Ilya Kosarev
> 
> 
> >Вторник, 12 ноября 2019, 18:32 +03:00 от Ilya Kosarev <i.kosarev@tarantool.org>:
> >
> >Hi!
> >
> >Thanks for your review!
> >Guess we might need to confirm it with Pettik 
> >or someone else who interacts with sql a lot,
> >but i am quite sure sqlRunParser doesn't need
> >to set diagnostics even in case of returning -1.
> >So far, i think we need to set diagnostics here,
> >in field_def_decode, like we do it in
> >ck_constraint_new.
> >Although it is not something i broke within
> >this patchset, i will add it to follow-up.
> >
> >Sincerely,
> >Ilya Kosarev
> >
> >
> >>Вторник, 12 ноября 2019, 14:00 +03:00 от Sergey Ostanevich < sergos@tarantool.org >:
> >>
> >>Hi!
> >>
> >>Thanks for the patch. One follow-up below, LGTM .
> >>
> >>
> >>Sergos
> >>
> >>>  	const char *dv = field->default_value;
> >>> @@ -496,8 +507,9 @@ field_def_decode(struct field_def *field, const char **data,
> >>>  		field->default_value_expr = sql_expr_compile(sql_get(), dv,
> >>>  							     strlen(dv));
> >>>  		if (field->default_value_expr == NULL)
> >>> -			diag_raise();
> >>> +			return -1;
> >>
> >>The sql_expr_compile() calls for sqlRunParser() that does not set diag
> >>in all exit paths. Please, put into followup.
> >>
> >>
> >
> >
> >-- 
> >Ilya Kosarev
> >field_def_decode
> 
> -- 
> Ilya Kosarev