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 91CCE2A505 for ; Wed, 20 Mar 2019 07:02:58 -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 sreKh3EG4jgT for ; Wed, 20 Mar 2019 07:02:58 -0400 (EDT) Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (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 1DB5528E3E for ; Wed, 20 Mar 2019 07:02: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> <6fc95943-2a4b-19f3-6b66-408e6d03bd7a@tarantool.org> From: Kirill Shcherbatov Message-ID: <26028559-c5ae-a594-b394-e4fbadf74b2b@tarantool.org> Date: Wed, 20 Mar 2019 14:02:56 +0300 MIME-Version: 1.0 In-Reply-To: <6fc95943-2a4b-19f3-6b66-408e6d03bd7a@tarantool.org> Content-Type: text/plain; charset=utf-8 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, Vladislav Shpilevoy > Thanks for the fixes. See 6 comments below. >> 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? It was not reasonable - here and in other places: it is the last function in this scope; but I've introduced "return" everywhere in parse.y; As for some other places, especially about patch "sql: refactor sql_expr_create to set diag" sql_expr_new usages in fk_constraint_action_trigger, there are many memory allocations so I don't like to manually release them in case of some failed allocation, so I've kept original logic. > >> 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. Fixed... >> + * 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. Okay. > 4. If you decided to completely refactor these functions,> then use enum on_conflict_action, please. Done. > >> + >> +/** >> + * 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. Done. > 6. On the branch I see a whitespace after '*' before 'table_name'. > Please, remove. Fixed. ====================================================== Refactored triggerSterAllocate routine as sql_trigger_step_new and reworked it to set diag_set in case of memory allocation error. Also performed some additional name refactoring in adjacent places. This change is necessary because the sql_trigger_step_allocate body has a sqlNormalizeName call that will be changed in subsequent patches. Part of #3931 --- src/box/sql/parse.y | 38 +++++++-- src/box/sql/sqlInt.h | 73 +++++++++++++++-- src/box/sql/trigger.c | 187 +++++++++++++++++------------------------- 3 files changed, 171 insertions(+), 127 deletions(-) diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index daeb6e84b..194af99b7 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -1430,20 +1430,42 @@ 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) { + pParse->is_aborted = true; + return; + } + } // INSERT -trigger_cmd(A) ::= insert_cmd(R) INTO trnm(X) idlist_opt(F) select(S). - {A = sqlTriggerInsertStep(pParse->db, &X, F, S, R);/*A-overwrites-R*/} +trigger_cmd(A) ::= insert_cmd(R) INTO trnm(X) idlist_opt(F) select(S). { + /*A-overwrites-R. */ + A = sql_trigger_insert_step(pParse->db, &X, F, S, R); + if (A == NULL) { + pParse->is_aborted = true; + return; + } +} // DELETE -trigger_cmd(A) ::= DELETE FROM trnm(X) tridxby where_opt(Y). - {A = sqlTriggerDeleteStep(pParse->db, &X, Y);} +trigger_cmd(A) ::= DELETE FROM trnm(X) tridxby where_opt(Y). { + A = sql_trigger_delete_step(pParse->db, &X, Y); + if (A == NULL) { + pParse->is_aborted = true; + return; + } +} // SELECT -trigger_cmd(A) ::= select(X). - {A = sqlTriggerSelectStep(pParse->db, X); /*A-overwrites-X*/} +trigger_cmd(A) ::= select(X). { + /* A-overwrites-X. */ + A = sql_trigger_select_step(pParse->db, X); + if (A == NULL) { + pParse->is_aborted = true; + return; + } +} // The special RAISE expression that may occur in trigger programs expr(A) ::= RAISE(X) LP IGNORE RP(Y). { diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 3aedde4cc..d3ffa9d9b 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -4109,12 +4109,73 @@ 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 select parameter points to) + * into a trigger step. + * 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. + * 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, + enum on_conflict_action orconf); + +/** + * Construct a trigger step that implements an UPDATE statemen. + * 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. The diag message is set. + */ +struct TriggerStep * +sql_trigger_update_step(struct sql *db, struct Token *table_name, + struct ExprList *new_list, struct Expr *where, + u8 orconf); + +/** + * Construct a trigger step that implements a DELETE statement. + * The parser calls this routine when it sees a DELETE statement + * inside the body of a CREATE TRIGGER. + * + * @param db The database connection. + * @param table_name The table from which rows are deleted. + * @param where The WHERE clause. + * @retval Not NULL TriggerStep object on success. + * @retval NULL Otherwise. The diag message is set. + */ +struct TriggerStep * +sql_trigger_delete_step(struct sql *db, struct Token *table_name, + struct Expr *where); /** * Triggers may access values stored in the old.* or new.* diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c index dc30c5f2c..544ba2a5a 100644 --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -248,142 +248,103 @@ cleanup: sqlDeleteTriggerStep(db, step_list); } -/* - * Turn a SELECT statement (that the pSelect parameter points to) into - * a trigger step. Return a pointer to a TriggerStep structure. - * - * The parser calls this routine when it finds a SELECT statement in - * body of a TRIGGER. - */ -TriggerStep * -sqlTriggerSelectStep(sql * db, Select * pSelect) +struct TriggerStep * +sql_trigger_select_step(struct sql *db, struct Select *select) { - TriggerStep *pTriggerStep = - sqlDbMallocZero(db, sizeof(TriggerStep)); - if (pTriggerStep == 0) { - sql_select_delete(db, pSelect); - return 0; + struct TriggerStep *trigger_step = + sqlDbMallocZero(db, sizeof(struct TriggerStep)); + if (trigger_step == NULL) { + sql_select_delete(db, select); + diag_set(OutOfMemory, sizeof(struct TriggerStep), + "sqlDbMallocZero", "trigger_step"); + return NULL; } - pTriggerStep->op = TK_SELECT; - pTriggerStep->pSelect = pSelect; - pTriggerStep->orconf = ON_CONFLICT_ACTION_DEFAULT; - return pTriggerStep; + trigger_step->op = TK_SELECT; + trigger_step->pSelect = select; + trigger_step->orconf = ON_CONFLICT_ACTION_DEFAULT; + return trigger_step; } /* * Allocate space to hold a new trigger step. The allocated space - * holds both the TriggerStep object and the TriggerStep.target.z string. + * holds both the TriggerStep object and the TriggerStep.target.z + * string. * - * If an OOM error occurs, NULL is returned and db->mallocFailed is set. + * @param db The database connection. + * @param op Trigger opcode. + * @param target_name The target name token. + * @retval Not NULL TriggerStep object on success. + * @retval NULL otherwise. The diag message is set. */ -static TriggerStep * -triggerStepAllocate(sql * db, /* Database connection */ - u8 op, /* Trigger opcode */ - Token * pName /* The target name */ - ) +static struct TriggerStep * +sql_trigger_step_new(struct sql *db, u8 op, struct Token *target_name) { - TriggerStep *pTriggerStep; - - pTriggerStep = - sqlDbMallocZero(db, sizeof(TriggerStep) + pName->n + 1); - if (pTriggerStep) { - char *z = (char *)&pTriggerStep[1]; - memcpy(z, pName->z, pName->n); - sqlNormalizeName(z); - pTriggerStep->zTarget = z; - pTriggerStep->op = op; + int size = sizeof(struct TriggerStep) + target_name->n + 1; + struct TriggerStep *trigger_step = sqlDbMallocZero(db, size); + if (trigger_step == NULL) { + diag_set(OutOfMemory, size, "sqlDbMallocZero", "trigger_step"); + return NULL; } - return pTriggerStep; + char *z = (char *)&trigger_step[1]; + memcpy(z, target_name->z, target_name->n); + sqlNormalizeName(z); + trigger_step->zTarget = z; + trigger_step->op = op; + return trigger_step; } -/* - * 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. - */ -TriggerStep * -sqlTriggerInsertStep(sql * db, /* The database connection */ - Token * pTableName, /* Name of the table into which we insert */ - IdList * pColumn, /* List of columns in pTableName to insert into */ - Select * pSelect, /* A SELECT statement that supplies values */ - u8 orconf /* The conflict algorithm - * (ON_CONFLICT_ACTION_ABORT, _REPLACE, - * etc.) - */ - ) +struct TriggerStep * +sql_trigger_insert_step(struct sql *db, struct Token *table_name, + struct IdList *column_list, struct Select *select, + enum on_conflict_action orconf) { - TriggerStep *pTriggerStep; - - assert(pSelect != 0 || db->mallocFailed); - - pTriggerStep = triggerStepAllocate(db, TK_INSERT, pTableName); - if (pTriggerStep) { - pTriggerStep->pSelect = - sqlSelectDup(db, pSelect, EXPRDUP_REDUCE); - pTriggerStep->pIdList = pColumn; - pTriggerStep->orconf = orconf; + assert(select != 0 || db->mallocFailed); + struct TriggerStep *trigger_step = + sql_trigger_step_new(db, TK_INSERT, table_name); + if (trigger_step != NULL) { + trigger_step->pSelect = + sqlSelectDup(db, select, EXPRDUP_REDUCE); + trigger_step->pIdList = column_list; + trigger_step->orconf = orconf; } else { - sqlIdListDelete(db, pColumn); + sqlIdListDelete(db, column_list); } - sql_select_delete(db, pSelect); - - return pTriggerStep; + sql_select_delete(db, select); + return trigger_step; } -/* - * 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. - */ -TriggerStep * -sqlTriggerUpdateStep(sql * db, /* The database connection */ - Token * pTableName, /* Name of the table to be updated */ - ExprList * pEList, /* The SET clause: list of column and new values */ - Expr * pWhere, /* The WHERE clause */ - u8 orconf /* The conflict algorithm. - * (ON_CONFLICT_ACTION_ABORT, _IGNORE, - * etc) - */ - ) +struct TriggerStep * +sql_trigger_update_step(struct sql *db, struct Token *table_name, + struct ExprList *new_list, struct Expr *where, + u8 orconf) { - TriggerStep *pTriggerStep; - - pTriggerStep = triggerStepAllocate(db, TK_UPDATE, pTableName); - if (pTriggerStep) { - pTriggerStep->pExprList = - sql_expr_list_dup(db, pEList, EXPRDUP_REDUCE); - pTriggerStep->pWhere = - sqlExprDup(db, pWhere, EXPRDUP_REDUCE); - pTriggerStep->orconf = orconf; + struct TriggerStep *trigger_step = + sql_trigger_step_new(db, TK_UPDATE, table_name); + if (trigger_step != NULL) { + trigger_step->pExprList = + sql_expr_list_dup(db, new_list, EXPRDUP_REDUCE); + trigger_step->pWhere = + sqlExprDup(db, where, EXPRDUP_REDUCE); + trigger_step->orconf = orconf; } - sql_expr_list_delete(db, pEList); - sql_expr_delete(db, pWhere, false); - return pTriggerStep; + sql_expr_list_delete(db, new_list); + sql_expr_delete(db, where, false); + return trigger_step; } -/* - * Construct a trigger step that implements a DELETE statement and return - * a pointer to that trigger step. The parser calls this routine when it - * sees a DELETE statement inside the body of a CREATE TRIGGER. - */ -TriggerStep * -sqlTriggerDeleteStep(sql * db, /* Database connection */ - Token * pTableName, /* The table from which rows are deleted */ - Expr * pWhere /* The WHERE clause */ - ) +struct TriggerStep * +sql_trigger_delete_step(struct sql *db, struct Token *table_name, + struct Expr *where) { - TriggerStep *pTriggerStep; - - pTriggerStep = triggerStepAllocate(db, TK_DELETE, pTableName); - if (pTriggerStep) { - pTriggerStep->pWhere = - sqlExprDup(db, pWhere, EXPRDUP_REDUCE); - pTriggerStep->orconf = ON_CONFLICT_ACTION_DEFAULT; + struct TriggerStep *trigger_step = + sql_trigger_step_new(db, TK_DELETE, table_name); + if (trigger_step != NULL) { + trigger_step->pWhere = + sqlExprDup(db, where, EXPRDUP_REDUCE); + trigger_step->orconf = ON_CONFLICT_ACTION_DEFAULT; } - sql_expr_delete(db, pWhere, false); - return pTriggerStep; + sql_expr_delete(db, where, false); + return trigger_step; } void -- 2.21.0