[tarantool-patches] Re: [PATCH v2 3/7] sql: refactor sql_src_list_append to set diag
Kirill Shcherbatov
kshcherbatov at tarantool.org
Mon Mar 11 18:04:50 MSK 2019
> 1. When you have a single goto error, usually you can
> just inline it. Besides, you do not goto error above on
> sql_alloc_src_list() fail. I understand why, but then it
> is not a common error.
>
> In other words, just inline error processing here.
Done.
> 2. sqlNameFromToken can fail even now, before next patches,
> and here it does not set diag, as I see, but the commit
> message says, that sql_src_list_append is now on diag.
I've described this in the commit message.
> 3. The same as 1.
Done.
> 4.1 The comment is obsolete in terms of variable names. Please,
> update it. You have no parameters 'pDatabase', 'pTable'.
> 4.2. Sentences and capital letters.
Done.
======================================================
Refactored sqlSrcListAppend routine as sql_src_list_append and
reworked it to use diag_set in case of memory allocation error.
This change is necessary because the sql_src_list_append body has
a sqlNameFromToken call that will be changed in subsequent
patches.
This patch refers to a series of preparatory patches that provide
the use of Tarantool errors in the call tree that includes
sqlNormalizeName, since this call can later return errors.
This patch is not self-sufficient, its sqlNameFromToken call
remained to be non-Tarantool (for now). It means, that if
sqlNameFromToken fails in sql_src_list_append there is no
diag message created.
Part of #3931
---
src/box/sql/build.c | 71 ++++++++++---------------------------
src/box/sql/delete.c | 14 ++++----
src/box/sql/fk_constraint.c | 17 +++++----
src/box/sql/parse.y | 24 ++++++++++---
src/box/sql/select.c | 8 ++---
src/box/sql/sqlInt.h | 22 +++++++++++-
src/box/sql/trigger.c | 11 +++---
7 files changed, 86 insertions(+), 81 deletions(-)
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index f53b2561c..04f65f165 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2652,64 +2652,26 @@ sql_src_list_new(struct sql *db)
return src_list;
}
-/*
- * Append a new table name to the given SrcList. Create a new SrcList if
- * need be. A new entry is created in the SrcList even if pTable is NULL.
- *
- * A SrcList is returned, or NULL if there is an OOM error. The returned
- * SrcList might be the same as the SrcList that was input or it might be
- * a new one. If an OOM error does occurs, then the prior value of pList
- * that is input to this routine is automatically freed.
- *
- * If pDatabase is not null, it means that the table has an optional
- * database name prefix. Like this: "database.table". The pDatabase
- * points to the table name and the pTable points to the database name.
- * The SrcList.a[].zName field is filled with the table name which might
- * come from pTable (if pDatabase is NULL) or from pDatabase.
- * SrcList.a[].zDatabase is filled with the database name from pTable,
- * or with NULL if no database is specified.
- *
- * In other words, if call like this:
- *
- * sqlSrcListAppend(D,A,B,0);
- *
- * Then B is a table name and the database name is unspecified. If called
- * like this:
- *
- * sqlSrcListAppend(D,A,B,C);
- *
- * Then C is the table name and B is the database name. If C is defined
- * then so is B. In other words, we never have a case where:
- *
- * sqlSrcListAppend(D,A,0,C);
- *
- * Both pTable and pDatabase are assumed to be quoted. They are dequoted
- * before being added to the SrcList.
- */
-SrcList *
-sqlSrcListAppend(sql * db, /* Connection to notify of malloc failures */
- SrcList * pList, /* Append to this SrcList. NULL creates a new SrcList */
- Token * pTable /* Table to append */
- )
+struct SrcList *
+sql_src_list_append(struct sql *db, struct SrcList *list,
+ struct Token *name_token)
{
- struct SrcList_item *pItem;
- assert(db != 0);
- if (pList == 0) {
- pList = sql_src_list_new(db);
- if (pList == 0)
- return 0;
+ if (list == NULL) {
+ list = sql_src_list_new(db);
+ if (list == NULL)
+ return NULL;
} else {
struct SrcList *new_list =
- sql_src_list_enlarge(db, pList, 1, pList->nSrc);
+ sql_src_list_enlarge(db, list, 1, list->nSrc);
if (new_list == NULL) {
- sqlSrcListDelete(db, pList);
+ sqlSrcListDelete(db, list);
return NULL;
}
- pList = new_list;
+ list = new_list;
}
- pItem = &pList->a[pList->nSrc - 1];
- pItem->zName = sqlNameFromToken(db, pTable);
- return pList;
+ struct SrcList_item *item = &list->a[list->nSrc - 1];
+ item->zName = sqlNameFromToken(db, name_token);
+ return list;
}
/*
@@ -2801,10 +2763,12 @@ sqlSrcListAppendFromTerm(Parse * pParse, /* Parsing context */
);
goto append_from_error;
}
- p = sqlSrcListAppend(db, p, pTable);
- if (p == 0 || NEVER(p->nSrc == 0)) {
+ p = sql_src_list_append(db, p, pTable);
+ if (p == NULL) {
+ sql_parser_error(pParse);
goto append_from_error;
}
+ assert(p->nSrc != 0);
pItem = &p->a[p->nSrc - 1];
assert(pAlias != 0);
if (pAlias->n) {
@@ -2821,6 +2785,7 @@ sqlSrcListAppendFromTerm(Parse * pParse, /* Parsing context */
sqlIdListDelete(db, pUsing);
sql_select_delete(db, pSubquery);
return 0;
+
}
/*
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 5170c7f59..26023554d 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -66,13 +66,15 @@ sql_materialize_view(struct Parse *parse, const char *name, struct Expr *where,
{
struct sql *db = parse->db;
where = sqlExprDup(db, where, 0);
- struct SrcList *from = sqlSrcListAppend(db, NULL, NULL);
- if (from != NULL) {
- assert(from->nSrc == 1);
- from->a[0].zName = sqlDbStrDup(db, name);
- assert(from->a[0].pOn == NULL);
- assert(from->a[0].pUsing == NULL);
+ struct SrcList *from = sql_src_list_append(db, NULL, NULL);
+ if (from == NULL) {
+ sql_parser_error(parse);
+ return;
}
+ assert(from->nSrc == 1);
+ from->a[0].zName = sqlDbStrDup(db, name);
+ assert(from->a[0].pOn == NULL);
+ assert(from->a[0].pUsing == NULL);
struct Select *select = sqlSelectNew(parse, NULL, from, where, NULL,
NULL, NULL, 0, NULL, NULL);
struct SelectDest dest;
diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c
index 4066b1cf1..d1eb6670a 100644
--- a/src/box/sql/fk_constraint.c
+++ b/src/box/sql/fk_constraint.c
@@ -618,9 +618,11 @@ fk_constraint_emit_check(struct Parse *parser, struct space *space, int reg_old,
* table. We need the child table as a SrcList for
* sqlWhereBegin().
*/
- struct SrcList *src = sqlSrcListAppend(db, NULL, NULL);
- if (src == NULL)
+ struct SrcList *src = sql_src_list_append(db, NULL, NULL);
+ if (src == NULL) {
+ sql_parser_error(parser);
continue;
+ }
struct SrcList_item *item = src->a;
struct space *child = space_by_id(fk->def->child_id);
assert(child != NULL);
@@ -866,11 +868,12 @@ fk_constraint_action_trigger(struct Parse *pParse, struct space_def *def,
"constraint failed");
if (r != NULL)
r->on_conflict_action = ON_CONFLICT_ACTION_ABORT;
- select = sqlSelectNew(pParse,
- sql_expr_list_append(db, NULL, r),
- sqlSrcListAppend(db, NULL, &err),
- where, NULL, NULL, NULL, 0, NULL,
- NULL);
+ struct SrcList *src_list = sql_src_list_append(db, NULL, &err);
+ if (src_list == NULL)
+ sql_parser_error(pParse);
+ select = sqlSelectNew(pParse, sql_expr_list_append(db, NULL, r),
+ src_list, where, NULL, NULL, NULL, 0,
+ NULL, NULL);
where = NULL;
}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 996f55d37..122fda2f0 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -609,8 +609,14 @@ seltablist(A) ::= stl_prefix(A) LP seltablist(F) RP
%type fullname {SrcList*}
%destructor fullname {sqlSrcListDelete(pParse->db, $$);}
-fullname(A) ::= nm(X).
- {A = sqlSrcListAppend(pParse->db,0,&X); /*A-overwrites-X*/}
+fullname(A) ::= nm(X). {
+ /*A-overwrites-X. */
+ A = sql_src_list_append(pParse->db,0,&X);
+ if (A == NULL) {
+ sql_parser_error(pParse);
+ return;
+ }
+}
%type joinop {int}
join_nm(A) ::= id(A).
@@ -1150,7 +1156,11 @@ expr(A) ::= expr(A) in_op(N) LP select(Y) RP(E). [IN] {
A.zEnd = &E.z[E.n];
}
expr(A) ::= expr(A) in_op(N) nm(Y) paren_exprlist(E). [IN] {
- SrcList *pSrc = sqlSrcListAppend(pParse->db, 0,&Y);
+ struct SrcList *pSrc = sql_src_list_append(pParse->db, 0,&Y);
+ if (pSrc == NULL) {
+ sql_parser_error(pParse);
+ return;
+ }
Select *pSelect = sqlSelectNew(pParse, 0,pSrc,0,0,0,0,0,0,0);
if( E ) sqlSrcListFuncArgs(pParse, pSelect ? pSrc : 0, E);
A.pExpr = sqlPExpr(pParse, TK_IN, A.pExpr, 0);
@@ -1220,8 +1230,12 @@ paren_exprlist(A) ::= LP exprlist(X) RP. {A = X;}
//
cmd ::= createkw(S) uniqueflag(U) INDEX ifnotexists(NE) nm(X)
ON nm(Y) LP sortlist(Z) RP. {
- sql_create_index(pParse, &X, sqlSrcListAppend(pParse->db,0,&Y), Z, &S,
- SORT_ORDER_ASC, NE, U);
+ struct SrcList *src_list = sql_src_list_append(pParse->db,0,&Y);
+ if (src_list == NULL) {
+ sql_parser_error(pParse);
+ return;
+ }
+ sql_create_index(pParse, &X, src_list, Z, &S, SORT_ORDER_ASC, NE, U);
}
%type uniqueflag {int}
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 840bca556..fda4296cc 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -256,7 +256,7 @@ findRightmost(Select * p)
/**
- * Work the same as sqlSrcListAppend(), but before adding to
+ * Work the same as sql_src_list_append(), but before adding to
* list provide check on name duplicates: only values with unique
* names are appended. Moreover, names of tables are not
* normalized: it is parser's business and in struct Select they
@@ -4116,9 +4116,9 @@ flattenSubquery(Parse * pParse, /* Parsing context */
} else {
assert(pParent != p); /* 2nd and subsequent times through the loop */
pSrc = pParent->pSrc =
- sqlSrcListAppend(db, 0, 0);
- if (pSrc == 0) {
- assert(db->mallocFailed);
+ sql_src_list_append(db, 0, 0);
+ if (pSrc == NULL) {
+ sql_parser_error(pParse);
break;
}
}
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 0895f7de1..2df1830d6 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3428,7 +3428,27 @@ sql_src_list_enlarge(struct sql *db, struct SrcList *src_list, int new_slots,
struct SrcList *
sql_src_list_new(struct sql *db);
-SrcList *sqlSrcListAppend(sql *, SrcList *, Token *);
+/**
+ * Append a new table name to the given list. Create a new
+ * SrcList if need be. A new entry is created in the list even
+ * if name_token is NULL.
+ *
+ * A SrcList is returned, or NULL if there is an OOM error.
+ * The returned SrcList might be the same as the list that was
+ * input or it might be a new one. If an OOM error does occurs,
+ * then the prior value of list that is input to this routine is
+ * automatically freed.
+ *
+ * @param db The database connection.
+ * @param list Append to this SrcList. NULL creates a new SrcList.
+ * @param name_token Token representing table name.
+ * @retval Not NULL SrcList pointer on success.
+ * @retval NULL Otherwise. The diag message is set.
+ */
+struct SrcList *
+sql_src_list_append(struct sql *db, struct SrcList *list,
+ struct Token *name_token);
+
SrcList *sqlSrcListAppendFromTerm(Parse *, SrcList *, Token *,
Token *, Select *, Expr *, IdList *);
void sqlSrcListIndexedBy(Parse *, SrcList *, Token *);
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index f7e6189de..20d41e6a1 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -588,12 +588,13 @@ targetSrcList(Parse * pParse, /* The parsing context */
sql *db = pParse->db;
SrcList *pSrc; /* SrcList to be returned */
- pSrc = sqlSrcListAppend(db, 0, 0);
- if (pSrc) {
- assert(pSrc->nSrc > 0);
- pSrc->a[pSrc->nSrc - 1].zName =
- sqlDbStrDup(db, pStep->zTarget);
+ pSrc = sql_src_list_append(db, 0, 0);
+ if (pSrc == NULL) {
+ sql_parser_error(pParse);
+ return NULL;
}
+ assert(pSrc->nSrc > 0);
+ pSrc->a[pSrc->nSrc - 1].zName = sqlDbStrDup(db, pStep->zTarget);
return pSrc;
}
--
2.21.0
More information about the Tarantool-patches
mailing list