From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 3E1A728355 for ; Mon, 18 Mar 2019 15:33:59 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zAcZ73LDshEk for ; Mon, 18 Mar 2019 15:33:59 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id DED6129796 for ; Mon, 18 Mar 2019 15:33:58 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 5/7] sql: refactor sql_trigger_step_allocate to set diag References: <607ffbbc4fcdca43ccfc5e6dabaed5ca3bc7719f.1551265819.git.kshcherbatov@tarantool.org> <403432db-e4b7-fc6f-73bb-13a98ec2959f@tarantool.org> From: Vladislav Shpilevoy Message-ID: <6fc95943-2a4b-19f3-6b66-408e6d03bd7a@tarantool.org> Date: Mon, 18 Mar 2019 22:33:56 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, Kirill Shcherbatov 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); > +