[tarantool-patches] Re: [PATCH v1 1/2] sql: restrict nullable action definitions

[Vladislav Shpilevoy]

Hello. Thanks for the fixes! See 3 comments below.

On 16/07/2018 12:43, Kirill Shcherbatov wrote:
>> I have pushed my review fixes on the branch, and they
>> correct some of the comments. Please, finish the patch
>> after my fixes including rest of the comments.
> Thank you for review, I've fixed all problems on branch and squashed.
> 
>> 1. Please, remove is_primkey. It is not needed here and can be easily
>> removed together with merging this cycle into the code below. The
>> only place where it is still used except here is
>> check_on_conflict_replace_entries, that can be spread over other
>> primary key checks.
>> 2. Please, use ER_NULLABLE_PRIMARY.
>> 3. This is checked already, so the rest of hunk below can be removed.
> Fixed with review fixes.

1. No, is_primkey still is not removed. I see it in struct Column.

> 
> 
> ============================================
> 
> 
> This patch dissallows define multiple "NULL", "NOT NULL"
> options per column and fixes silent implicit behavior
> for invalid "NULL PRIMARY KEY" construction.
> 
> Closes #3473.
> ---
>   src/box/sql/build.c           | 86 +++++++++++++++++++++++++++++++++----------
>   src/box/sql/parse.y           |  7 +++-
>   test/sql/on-conflict.result   | 21 +++++++++++
>   test/sql/on-conflict.test.lua |  8 ++++
>   4 files changed, 101 insertions(+), 21 deletions(-)
> 
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index ac935fd..e2bdc4a 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -276,7 +276,12 @@ ccons ::= DEFAULT id(X).              {
>   // In addition to the type name, we also care about the primary key and
>   // UNIQUE constraints.
>   //
> -ccons ::= NULL onconf.
> +ccons ::= NULL onconf(R).        {
> +    sqlite3AddNotNull(pParse, ON_CONFLICT_ACTION_NONE);
> +    /* Trigger nullability mismatch error if required. */
> +    if (R != ON_CONFLICT_ACTION_DEFAULT)

2. Why do you need this check?

3. Why sqlite3AddNotNull is called when NULL is allowed?
Please, rename the function or split it or something.

> +        sqlite3AddNotNull(pParse, R);
> +}
>   ccons ::= NOT NULL onconf(R).    {sqlite3AddNotNull(pParse, R);}
>   ccons ::= PRIMARY KEY sortorder(Z) onconf(R) autoinc(I).
>                                    {sqlite3AddPrimaryKey(pParse,0,R,I,Z);}