[tarantool-patches] Re: [PATCH v2 5/7] sql: refactor sql_trigger_step_allocate to set diag
Kirill Shcherbatov
kshcherbatov at tarantool.org
Wed Mar 20 14:02:56 MSK 2019
> 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
More information about the Tarantool-patches
mailing list