[tarantool-patches] [PATCH v2 3/7] sql: refactor sql_src_list_append to set diag

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

Refactored sql_src_list_append routine 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.

Needed for #3931
---
 src/box/sql/build.c         | 83 +++++++++++--------------------------
 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        | 48 ++++++++++++++++++++-
 src/box/sql/trigger.c       | 11 ++---
 7 files changed, 119 insertions(+), 86 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 5337df450..4fe838608 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2654,64 +2654,28 @@ sql_alloc_src_list(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_alloc_src_list(db);
-		if (pList == 0)
-			return 0;
+	if (list == NULL) {
+		list = sql_alloc_src_list(db);
+		if (list == NULL)
+			return NULL;
 	} else {
 		struct SrcList *new_list =
-			sql_src_list_enlarge(db, pList, 1, pList->nSrc);
-		if (new_list == NULL) {
-			sqlSrcListDelete(db, pList);
-			return NULL;
-		}
-		pList = new_list;
-	}
-	pItem = &pList->a[pList->nSrc - 1];
-	pItem->zName = sqlNameFromToken(db, pTable);
-	return pList;
+			sql_src_list_enlarge(db, list, 1, list->nSrc);
+		if (new_list == NULL)
+			goto error;
+		list = new_list;
+	}
+	pItem = &list->a[list->nSrc - 1];
+	pItem->zName = sqlNameFromToken(db, name_token);
+	return list;
+error:
+	sqlSrcListDelete(db, list);
+	return NULL;
 }
 
 /*
@@ -2803,10 +2767,10 @@ sqlSrcListAppendFromTerm(Parse * pParse,	/* Parsing context */
 		    );
 		goto append_from_error;
 	}
-	p = sqlSrcListAppend(db, p, pTable);
-	if (p == 0 || NEVER(p->nSrc == 0)) {
-		goto append_from_error;
-	}
+	p = sql_src_list_append(db, p, pTable);
+	if (p == NULL)
+		goto tnt_error;
+	assert(p->nSrc != 0);
 	pItem = &p->a[p->nSrc - 1];
 	assert(pAlias != 0);
 	if (pAlias->n) {
@@ -2823,6 +2787,9 @@ sqlSrcListAppendFromTerm(Parse * pParse,	/* Parsing context */
 	sqlIdListDelete(db, pUsing);
 	sql_select_delete(db, pSubquery);
 	return 0;
+tnt_error:
+	sql_parser_error(pParse);
+	goto append_from_error;
 }
 
 /*
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 93c01a0e7..4f761d610 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 32db6850d..4ae9eff83 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).
@@ -1146,7 +1152,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);
@@ -1216,8 +1226,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 0baf8fb57..7eea45dd7 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 ea6476931..eae2ec8e8 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3462,7 +3462,53 @@ sql_src_list_enlarge(struct sql *db, struct SrcList *src_list, int new_slots,
 struct SrcList *
 sql_alloc_src_list(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 SrcList 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.
+ *
+ * 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
+ * name_token, or with NULL if no database is specified.
+ *
+ * In other words, if call like this:
+ *
+ *         sql_src_list_append(D,A,B,0);
+ *
+ * Then B is a table name and the database name is unspecified.
+ * If called like this:
+ *
+ *         sql_src_list_append(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:
+ *
+ *         sql_src_list_append(D,A,0,C);
+ *
+ * Both pTable and pDatabase are assumed to be quoted. They are
+ * dequoted before being added to the SrcList.
+ * @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.
+ */
+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.20.1