[tarantool-patches] Re: [PATCH 1/6] sql: move constraint name to struct contraint_parse

[Vladislav Shpilevoy]

Hi! Thanks for the fixes! See 2 comments below.

> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 51a5d01b5..981e3c660 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2697,7 +2697,7 @@ struct fkey_parse {
>    */
>   struct constraint_parse {
>          /** Name of table which constraint belongs to. */
> -       struct SrcList *table_name;
> +       struct SrcList *table;
>          /** Name of the constraint currently being parsed. */
>          struct Token name;
>   };
> 
>>
>>> +	/** Name of the constraint currently being parsed. */
>>> +	struct Token name;
>>> +};
>>
>> 2.1. Also, I see that struct constraint_parse is not able to
>> describe a foreign key - it has no parent table, referenced
>> columns - you pass them separately from struct constraint_parse
>> which looks contr-intuitive. Can you please, elaborate so it,
>> for example, has struct fkey_parse as a member? Or at least,
>> have both parent and child table name and cols as Token and
>> ExprList pointers?
> 
> According to ANSI only three constraints can be added:
> UNIQUE/PK, FK and CHECK.

1. But you've named foreign key system space _fk_constraint ...
And here you have struct constraint_parse, which is not able to
fit a whole fk *constraint*.

> Only name of table and name
> of constraint are shared among features of these constraints:
> CHECK also has ExprSpan and UNIQUE - columns (child).
> I guess it is fair to move child cols to this structure,
> but other members seems to be redundant.
> Hence, I will add parent name and parent colls
> only if you insist.

If I were you I would add here both child and parent attributes,
is_deferred, action, but who am I to insist on anything? Please,
consult server team chat whether we should consider foreign keys
as constraints. Or do nothing. I am not a manager, so it is just
an advice. I do not insist on elaborating this structure further.
(Although it looks unfinished to me.)

> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 963b16626..af961592a 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> 
> @@ -727,8 +726,10 @@ sqlite3AddPrimaryKey(Parse * pParse,       /* Parsing context */
>                                                               &token, 0));
>                  if (list == NULL)
>                          goto primary_key_exit;
> -               sql_create_index(pParse, 0, 0, list, 0, SORT_ORDER_ASC,
> +               pParse->constraint.cols = list;
> +               sql_create_index(pParse, 0, 0, 0, SORT_ORDER_ASC,
>                                   false, SQL_INDEX_TYPE_CONSTRAINT_PK);
> +               pParse->constraint.cols = NULL;

2. I think, that it is safer to nullify pParse->constraint.cols in the
same place where it is deleted, inside sql_create_index. What do you think?
Also, it removes code duplication here and in the hunk below.

>                  if (db->mallocFailed)
>                          goto primary_key_exit;
>          } else if (autoInc) {
> @@ -736,8 +737,10 @@ sqlite3AddPrimaryKey(Parse * pParse,       /* Parsing context */
>                                  "INTEGER PRIMARY KEY or INT PRIMARY KEY");
>                  goto primary_key_exit;
>          } else {
> -               sql_create_index(pParse, 0, 0, pList, 0, sortOrder, false,
> +               pParse->constraint.cols = pList;
> +               sql_create_index(pParse, 0, 0, 0, sortOrder, false,
>                                   SQL_INDEX_TYPE_CONSTRAINT_PK);
> +               pParse->constraint.cols = NULL;
>                  pList = 0;
>                  if (pParse->nErr > 0)
>                          goto primary_key_exit;
>