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

[Vladislav Shpilevoy]

Hi! Thanks for the patch! See 3 comments below.

On 09/01/2019 15:13, Nikita Pettik wrote:
> Before this patch name of constraint being parsed was held as a separate
> struct Token within parser context. Meanwhile, we also need to store
> name of table which constraint is related to (in order to implement
> ALTER TABLE ADD CONSTRAINT with several options: foreign key, unique,
> primary key, etc). Hence, lets move constraint name to a separate
> structure and save it alongside with the name of table.
> 
> Needed for #3097
> ---
>   src/box/sql/alter.c     |  4 ++--
>   src/box/sql/build.c     | 36 +++++++++++++++++++-----------------
>   src/box/sql/parse.y     | 22 +++++++++++++---------
>   src/box/sql/prepare.c   | 11 +++++++++++
>   src/box/sql/sqliteInt.h | 32 ++++++++++++++++++--------------
>   5 files changed, 63 insertions(+), 42 deletions(-)
> 
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 4110a5991..51a5d01b5 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2691,6 +2691,17 @@ struct fkey_parse {
>   	struct rlist link;
>   };
>   
> +/**
> + * Used to hold intermediate meta-information during
> + * constraint creation or alteration.
> + */
> +struct constraint_parse {
> +	/** Name of table which constraint belongs to. */
> +	struct SrcList *table_name;

1. I guess, it is not a 'table_name', but a 'table' since it
is struct SrcList, not const char * nor Token.

> +	/** 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?

> +
>   /*
>    * An SQL parser context.  A copy of this structure is passed through
>    * the parser and down into all the parser action routine in order to
> @@ -2781,6 +2791,10 @@ struct Parse {
>   	TriggerPrg *pTriggerPrg;	/* Linked list of coded triggers */
>   	With *pWith;		/* Current WITH clause, or NULL */
>   	With *pWithToFree;	/* Free this WITH object at the end of the parse */
> +	/**
> +	 * Constraint currently being parsed.
> +	 */
> +	struct constraint_parse *constraint;

3. Make it an object, not a pointer, please. Anyway you allocate it on
each prepare.

>   	/**
>   	 * Number of FK constraints declared within
>   	 * CREATE TABLE statement.
> @@ -4152,8 +4161,7 @@ fkey_change_defer_mode(struct Parse *parse_context, bool is_deferred);
>    *                algorithms (e.g. CASCADE, RESTRICT etc).
>    */
>   void
> -sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child,
> -		       struct Token *constraint, struct ExprList *child_cols,
> +sql_create_foreign_key(struct Parse *parse_context, struct ExprList *child_cols,
>   		       struct Token *parent, struct ExprList *parent_cols,
>   		       bool is_deferred, int actions);

2.2 This is what I've described in point 2 - child part is partially moved into
struct contraint_parse, whilst parent still is separate.