[tarantool-patches] Re: [PATCH v2 5/7] sql: refactor sql_trigger_step_allocate to set diag

[Vladislav Shpilevoy]

Thanks for the fixes. See 6 comments below.

> 
> Part of #3931
> ---
>   src/box/sql/parse.y   |  30 +++++--
>   src/box/sql/sqlInt.h  |  77 +++++++++++++++--
>   src/box/sql/trigger.c | 187 +++++++++++++++++-------------------------
>   3 files changed, 167 insertions(+), 127 deletions(-)
> 
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 8a8f1b82f..f0388b481 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -1415,20 +1415,34 @@ tridxby ::= NOT INDEXED. {
>   %destructor trigger_cmd {sqlDeleteTriggerStep(pParse->db, $$);}
>   // UPDATE
>   trigger_cmd(A) ::=
> -   UPDATE orconf(R) trnm(X) tridxby SET setlist(Y) where_opt(Z).
> -   {A = sqlTriggerUpdateStep(pParse->db, &X, Y, Z, R);}
> +   UPDATE orconf(R) trnm(X) tridxby SET setlist(Y) where_opt(Z). {
> +     A = sql_trigger_update_step(pParse->db, &X, Y, Z, R);
> +     if (A == NULL)
> +        sql_parser_error(pParse);

1. Why do not you return after an error here and in other places?

> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index b49229b26..4e6c5717b 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -4087,12 +4087,77 @@ vdbe_code_row_trigger_direct(struct Parse *parser, struct sql_trigger *trigger,
>   			     int ignore_jump);
>   
>   void sqlDeleteTriggerStep(sql *, TriggerStep *);
> -TriggerStep *sqlTriggerSelectStep(sql *, Select *);
> -TriggerStep *sqlTriggerInsertStep(sql *, Token *, IdList *,
> -				      Select *, u8);
> -TriggerStep *sqlTriggerUpdateStep(sql *, Token *, ExprList *, Expr *,
> -				      u8);
> -TriggerStep *sqlTriggerDeleteStep(sql *, Token *, Expr *);
> +
> +/**
> + * Turn a SELECT statement (that the pSelect parameter points to)

2. Again old names.

> + * into a trigger step. Return a pointer to a TriggerStep
> + * structure.

3. In that case a type of return value is strictly determined
thanks to C language. Not sure if that sentence makes a sense.
The same about other places.

> + * The parser calls this routine when it finds a SELECT statement
> + * in body of a TRIGGER.
> + *
> + * @param db The database connection.
> + * @param select The SELECT statement to process.
> + * @retval Not NULL TriggerStep object on success.
> + * @retval NULL Otherwise. The diag message is set.
> + */
> +struct TriggerStep *
> +sql_trigger_select_step(struct sql *db, struct Select *select);
> +
> +/**
> + * Build a trigger step out of an INSERT statement. Return a
> + * pointer to the new trigger step.
> + * The parser calls this routine when it sees an INSERT inside the
> + * body of a trigger.
> + *
> + * @param db The database connection.
> + * @param table_name Name of the table into which we insert.
> + * @param column_list List of columns in table to insert into.
> + * @param select The SELECT statement that supplies values.
> + * @param orconf The conflict algorithm
> + *               (ON_CONFLICT_ACTION_ABORT, _REPLACE, etc.).
> + * @retval Not NULL TriggerStep object on success.
> + * @retval NULL Otherwise. The diag message is set.
> + */
> +struct TriggerStep *
> +sql_trigger_insert_step(struct sql *db, struct Token *table_name,
> +			struct IdList *column_list, struct Select *select,
> +			u8 orconf);

4. If you decided to completely refactor these functions,
then use enum on_conflict_action, please.

> +
> +/**
> + * Construct a trigger step that implements an UPDATE statement
> + * and return a pointer to that trigger step. The parser calls
> + * this routine when it sees an UPDATE statement inside the body
> + * of a CREATE TRIGGER.
> + *
> + * @param db The database connection.
> + * @param table_name Name of the table to be updated.
> + * @param new_list The SET clause: list of column and new values.
> + * @param where The WHERE clause.
> + * @param orconf The conflict algorithm
> + *               (ON_CONFLICT_ACTION_ABORT, _REPLACE, etc.).
> + * @retval Not NULL TriggerStep object on success.
> + * @retval NULL Otherwise.

5. In other places you wrote 'The diag message is set.'. Is it
set here? I know, that it is, but the comment differs from the others.
We should either never say about diag explicitly, or say it everywhere.
And I think that now, while some SQL functions do not set diag, we
should say about diag explicitly.

> + */
> +struct TriggerStep *
> +sql_trigger_update_step(struct sql *db, struct Token *table_name,

6. On the branch I see a whitespace after '*' before 'table_name'.
Please, remove.

> +		        struct ExprList *new_list, struct Expr *where,
> +			u8 orconf);
> +