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