[tarantool-patches] Re: [PATCH v2 4/7] sql: refactor sql_name_from_token to set diag

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

On 07.03.2019 20:34, Vladislav Shpilevoy wrote:
> See 5 comments.First of all, I've split this patch on two patches.

> 1. You already have the same assertion in sql_name_from_token.
> Why do you need so many new equal assertions? Check other places
> like build.c:1792 by yourself, please.
I've dropped them all.

> 2. Double check for zName == NULL.
Fixed.

> 3. You can make it shorter writing
> 
>      } else if (pValue != NULL) {
>          ...
>      }
> 
> Instead of
> 
>      } else {
>          if (pValue != NULL) {
>              ...
>          }
>      }

Ok.

> 4. The last sentence is exactly what you wrote after @retvals.
> Please, do not duplicate it.
I've fixed it here.

> 5. Sentences are started from capital letters. I will not write
> that again. Please, find and fix other places.
I've really looked over the patch for this.

=========================================================

Refactored sqlIdListAppend routine as sql_id_list_append and
reworked it to use diag_set in case of memory allocation error.
This change is necessary because the sql_id_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_id_list_append there is no
diag message created.

Part of #3931
---
 src/box/sql/build.c  | 44 +++++++++++++++++++++-----------------------
 src/box/sql/expr.c   |  2 +-
 src/box/sql/parse.y  | 15 +++++++++++----
 src/box/sql/sqlInt.h | 15 ++++++++++++++-
 4 files changed, 47 insertions(+), 29 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 04f65f165..d374acb47 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2538,30 +2538,28 @@ sqlArrayAllocate(sql * db,	/* Connection to notify of malloc failures */
 	return pArray;
 }
 
-/*
- * Append a new element to the given IdList.  Create a new IdList if
- * need be.
- *
- * A new IdList is returned, or NULL if malloc() fails.
- */
-IdList *
-sqlIdListAppend(sql * db, IdList * pList, Token * pToken)
-{
-	int i;
-	if (pList == 0) {
-		pList = sqlDbMallocZero(db, sizeof(IdList));
-		if (pList == 0)
-			return 0;
-	}
-	pList->a = sqlArrayAllocate(db,
-					pList->a,
-					sizeof(pList->a[0]), &pList->nId, &i);
-	if (i < 0) {
-		sqlIdListDelete(db, pList);
-		return 0;
+struct IdList *
+sql_id_list_append(struct sql *db, struct IdList *list,
+		   struct Token *name_token)
+{
+	if (list == NULL &&
+	    (list = sqlDbMallocZero(db, sizeof(IdList))) == NULL) {
+		diag_set(OutOfMemory, sizeof(IdList), "sqlDbMallocZero",
+			 "list");
+		return NULL;
 	}
-	pList->a[i].zName = sqlNameFromToken(db, pToken);
-	return pList;
+	int i;
+	list->a = sqlArrayAllocate(db, list->a, sizeof(list->a[0]),
+				   &list->nId, &i);
+	if (i < 0)
+		goto error;
+	list->a[i].zName = sqlNameFromToken(db, name_token);
+	if (list->a[i].zName == NULL)
+		goto error;
+	return list;
+error:
+	sqlIdListDelete(db, list);
+	return NULL;
 }
 
 /*
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index a75f23756..44b6ce11f 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1638,7 +1638,7 @@ sqlIdListDup(sql * db, IdList * p)
 		return 0;
 	}
 	/* Note that because the size of the allocation for p->a[] is not
-	 * necessarily a power of two, sqlIdListAppend() may not be called
+	 * necessarily a power of two, sql_id_list_append() may not be called
 	 * on the duplicate created by this function.
 	 */
 	for (i = 0; i < p->nId; i++) {
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 122fda2f0..8a8f1b82f 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -797,10 +797,17 @@ insert_cmd(A) ::= REPLACE.            {A = ON_CONFLICT_ACTION_REPLACE;}
 
 idlist_opt(A) ::= .                       {A = 0;}
 idlist_opt(A) ::= LP idlist(X) RP.    {A = X;}
-idlist(A) ::= idlist(A) COMMA nm(Y).
-    {A = sqlIdListAppend(pParse->db,A,&Y);}
-idlist(A) ::= nm(Y).
-    {A = sqlIdListAppend(pParse->db,0,&Y); /*A-overwrites-Y*/}
+idlist(A) ::= idlist(A) COMMA nm(Y). {
+  A = sql_id_list_append(pParse->db,A,&Y);
+  if (A == NULL)
+    sql_parser_error(pParse);
+}
+idlist(A) ::= nm(Y). {
+  /* A-overwrites-Y. */
+  A = sql_id_list_append(pParse->db,0,&Y);
+  if (A == NULL)
+    sql_parser_error(pParse);
+}
 
 /////////////////////////// Expression Processing /////////////////////////////
 //
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 2df1830d6..56ae90a95 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3388,7 +3388,20 @@ sql_drop_table(struct Parse *, struct SrcList *, bool, bool);
 void sqlInsert(Parse *, SrcList *, Select *, IdList *,
 	       enum on_conflict_action);
 void *sqlArrayAllocate(sql *, void *, int, int *, int *);
-IdList *sqlIdListAppend(sql *, IdList *, Token *);
+
+/**
+ * Append a new element to the given IdList. Create a new IdList
+ * if need be.
+ *
+ * @param db The database connection.
+ * @param list The pointer to existent Id list if exists.
+ * @param name_token The token containing name.
+ * @retval Not NULL IdList pointer is returned on success.
+ * @retval NULL otherwise. Diag message is set.
+ */
+struct IdList *
+sql_id_list_append(struct sql *db, struct IdList *pList, struct Token *pToken);
+
 int sqlIdListIndex(IdList *, const char *);
 
 /**
-- 
2.21.0

===========================================================

Refactored sqlNameFromToken routine as sql_name_from_token and
reworked it to use diag_set in case of memory allocation error.
This change is necessary because the sql_name_from_token body has
a sqlNameFromToken call that will be changed in subsequent
patches.

Part of #3931
---
 src/box/sql/alter.c   |   7 +-
 src/box/sql/analyze.c |  47 +++++-----
 src/box/sql/build.c   | 206 +++++++++++++++++++++++-------------------
 src/box/sql/pragma.c  |  24 +++--
 src/box/sql/sqlInt.h  |  21 ++++-
 src/box/sql/trigger.c |   4 +-
 6 files changed, 183 insertions(+), 126 deletions(-)

diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index d49ebb8df..3a9e9095e 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -43,9 +43,9 @@ sql_alter_table_rename(struct Parse *parse, struct SrcList *src_tab,
 {
 	assert(src_tab->nSrc == 1);
 	struct sql *db = parse->db;
-	char *new_name = sqlNameFromToken(db, new_name_tk);
+	char *new_name = sql_name_from_token(db, new_name_tk);
 	if (new_name == NULL)
-		goto exit_rename_table;
+		goto tnt_error;
 	/* Check that new name isn't occupied by another table. */
 	if (space_by_name(new_name) != NULL) {
 		diag_set(ClientError, ER_SPACE_EXISTS, new_name);
@@ -72,8 +72,7 @@ exit_rename_table:
 	return;
 tnt_error:
 	sqlDbFree(db, new_name);
-	parse->rc = SQL_TARANTOOL_ERROR;
-	parse->nErr++;
+	sql_parser_error(parse);
 	goto exit_rename_table;
 }
 
diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 8c83288e6..cf5af7a8b 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -1111,34 +1111,39 @@ vdbe_emit_analyze_table(struct Parse *parse, struct space *space)
 void
 sqlAnalyze(Parse * pParse, Token * pName)
 {
-	sql *db = pParse->db;
+	char *name = NULL;
+	struct sql *db = pParse->db;
+	struct Vdbe *v = sqlGetVdbe(pParse);
+	if (v == NULL)
+		return;
 	if (pName == NULL) {
 		/* Form 1:  Analyze everything */
 		sql_analyze_database(pParse);
 	} else {
 		/* Form 2:  Analyze table named */
-		char *z = sqlNameFromToken(db, pName);
-		if (z != NULL) {
-			struct space *sp = space_by_name(z);
-			if (sp != NULL) {
-				if (sp->def->opts.is_view) {
-					sqlErrorMsg(pParse, "VIEW isn't "\
-							"allowed to be "\
-							"analyzed");
-				} else {
-					vdbe_emit_analyze_table(pParse, sp);
-				}
-			} else {
-				diag_set(ClientError, ER_NO_SUCH_SPACE, z);
-				pParse->rc = SQL_TARANTOOL_ERROR;
-				pParse->nErr++;
-			}
-			sqlDbFree(db, z);
+		name = sql_name_from_token(db, pName);
+		if (name == NULL)
+			goto tnt_error;
+		struct space *sp = space_by_name(name);
+		if (sp == NULL) {
+			diag_set(ClientError, ER_NO_SUCH_SPACE, name);
+			goto tnt_error;
+		}
+		if (sp->def->opts.is_view) {
+			sqlErrorMsg(pParse,
+				    "VIEW isn't allowed to be analyzed");
+			goto cleanup;
+		} else {
+			vdbe_emit_analyze_table(pParse, sp);
 		}
 	}
-	Vdbe *v = sqlGetVdbe(pParse);
-	if (v != NULL)
-		sqlVdbeAddOp0(v, OP_Expire);
+	sqlVdbeAddOp0(v, OP_Expire);
+cleanup:
+	sqlDbFree(db, name);
+	return;
+tnt_error:
+	sql_parser_error(pParse);
+	goto cleanup;
 }
 
 ssize_t
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index d374acb47..1864bd0ad 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -232,30 +232,18 @@ sql_space_column_is_in_pk(struct space *space, uint32_t column)
 	return false;
 }
 
-/*
- * Given a token, return a string that consists of the text of that
- * token.  Space to hold the returned string
- * is obtained from sqlMalloc() and must be freed by the calling
- * function.
- *
- * Any quotation marks (ex:  "name", 'name', [name], or `name`) that
- * surround the body of the token are removed.
- *
- * Tokens are often just pointers into the original SQL text and so
- * are not \000 terminated and are not persistent.  The returned string
- * is \000 terminated and is persistent.
- */
 char *
-sqlNameFromToken(sql * db, Token * pName)
+sql_name_from_token(struct sql *db, struct Token *name_token)
 {
-	char *zName;
-	if (pName) {
-		zName = sqlDbStrNDup(db, (char *)pName->z, pName->n);
-		sqlNormalizeName(zName);
-	} else {
-		zName = 0;
+	assert(name_token != NULL && name_token->z != NULL);
+	char *name = sqlDbStrNDup(db, (char *)name_token->z, name_token->n);
+	if (name == NULL) {
+		diag_set(OutOfMemory, name_token->n + 1, "sqlDbStrNDup",
+			 "name");
+		return NULL;
 	}
-	return zName;
+	sqlNormalizeName(name);
+	return name;
 }
 
 /*
@@ -332,11 +320,11 @@ sqlStartTable(Parse *pParse, Token *pName, int noErr)
 		goto cleanup;
 	sqlVdbeCountChanges(v);
 
-	zName = sqlNameFromToken(db, pName);
+	zName = sql_name_from_token(db, pName);
+	if (zName == NULL)
+		goto tnt_error;
 
 	pParse->sNameToken = *pName;
-	if (zName == 0)
-		return;
 	if (sqlCheckIdentifierName(pParse, zName) != SQL_OK)
 		goto cleanup;
 
@@ -344,10 +332,9 @@ sqlStartTable(Parse *pParse, Token *pName, int noErr)
 	if (space != NULL) {
 		if (!noErr) {
 			diag_set(ClientError, ER_SPACE_EXISTS, zName);
-			sql_parser_error(pParse);
-		} else {
-			assert(!db->init.busy || CORRUPT_DB);
+			goto tnt_error;
 		}
+		assert(!db->init.busy || CORRUPT_DB);
 		goto cleanup;
 	}
 
@@ -367,6 +354,9 @@ sqlStartTable(Parse *pParse, Token *pName, int noErr)
  cleanup:
 	sqlDbFree(db, zName);
 	return;
+tnt_error:
+	sql_parser_error(pParse);
+	goto cleanup;
 }
 
 /**
@@ -708,11 +698,13 @@ sqlAddCollateType(Parse * pParse, Token * pToken)
 	struct space *space = pParse->new_space;
 	uint32_t i = space->def->field_count - 1;
 	sql *db = pParse->db;
-	char *zColl = sqlNameFromToken(db, pToken);
-	if (!zColl)
+	char *coll_name = sql_name_from_token(db, pToken);
+	if (coll_name == NULL) {
+		sql_parser_error(pParse);
 		return;
+	}
 	uint32_t *coll_id = &space->def->fields[i].coll_id;
-	if (sql_get_coll_seq(pParse, zColl, coll_id) != NULL) {
+	if (sql_get_coll_seq(pParse, coll_name, coll_id) != NULL) {
 		/* If the column is declared as "<name> PRIMARY KEY COLLATE <type>",
 		 * then an index may have been created on this column before the
 		 * collation type was added. Correct this if it is the case.
@@ -726,7 +718,7 @@ sqlAddCollateType(Parse * pParse, Token * pToken)
 			}
 		}
 	}
-	sqlDbFree(db, zColl);
+	sqlDbFree(db, coll_name);
 }
 
 struct coll *
@@ -1749,10 +1741,9 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child,
 		memset(fk_parse, 0, sizeof(*fk_parse));
 		rlist_add_entry(&parse_context->new_fk_constraint, fk_parse, link);
 	}
-	assert(parent != NULL);
-	parent_name = sqlNameFromToken(db, parent);
+	parent_name = sql_name_from_token(db, parent);
 	if (parent_name == NULL)
-		goto exit_create_fk;
+		goto tnt_error;
 	/*
 	 * Within ALTER TABLE ADD CONSTRAINT FK also can be
 	 * self-referenced, but in this case parent (which is
@@ -1785,15 +1776,19 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child,
 				sqlMPrintf(db, "FK_CONSTRAINT_%d_%s",
 					       ++parse_context->fk_constraint_count,
 					       space->def->name);
+			if (constraint_name == NULL)
+				goto exit_create_fk;
 		} else {
 			struct Token *cnstr_nm = &parse_context->constraintName;
-			constraint_name = sqlNameFromToken(db, cnstr_nm);
+			constraint_name = sql_name_from_token(db, cnstr_nm);
+			if (constraint_name == NULL)
+				goto tnt_error;
 		}
 	} else {
-		constraint_name = sqlNameFromToken(db, constraint);
+		constraint_name = sql_name_from_token(db, constraint);
+		if (constraint_name == NULL)
+			goto tnt_error;
 	}
-	if (constraint_name == NULL)
-		goto exit_create_fk;
 	const char *error_msg = "number of columns in foreign key does not "
 				"match the number of columns in the primary "
 				"index of referenced table";
@@ -1922,15 +1917,17 @@ sql_drop_foreign_key(struct Parse *parse_context, struct SrcList *table,
 	struct space *child = space_by_name(table_name);
 	if (child == NULL) {
 		diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
-		parse_context->rc = SQL_TARANTOOL_ERROR;
-		parse_context->nErr++;
+		sql_parser_error(parse_context);
+		return;
+	}
+	char *constraint_name =
+		sql_name_from_token(parse_context->db, constraint);
+	if (constraint_name == NULL) {
+		sql_parser_error(parse_context);
 		return;
 	}
-	char *constraint_name = sqlNameFromToken(parse_context->db,
-						     constraint);
-	if (constraint_name != NULL)
-		vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
-				    child->def->id);
+	vdbe_emit_fk_constraint_drop(parse_context, constraint_name,
+				     child->def->id);
 	/*
 	 * We account changes to row count only if drop of
 	 * foreign keys take place in a separate
@@ -2192,9 +2189,9 @@ sql_create_index(struct Parse *parse, struct Token *token,
 	 */
 	if (token != NULL) {
 		assert(token->z != NULL);
-		name = sqlNameFromToken(db, token);
+		name = sql_name_from_token(db, token);
 		if (name == NULL)
-			goto exit_create_index;
+			goto tnt_error;
 		if (sql_space_index_by_name(space, name) != NULL) {
 			if (!if_not_exist) {
 				diag_set(ClientError, ER_INDEX_EXISTS_IN_SPACE,
@@ -2205,10 +2202,13 @@ sql_create_index(struct Parse *parse, struct Token *token,
 		}
 	} else {
 		char *constraint_name = NULL;
-		if (parse->constraintName.z != NULL)
+		if (parse->constraintName.z != NULL) {
 			constraint_name =
-				sqlNameFromToken(db,
-						     &parse->constraintName);
+				sql_name_from_token(db,
+						    &parse->constraintName);
+			if (constraint_name == NULL)
+				goto tnt_error;
+		}
 
 	       /*
 		* This naming is temporary. Now it's not
@@ -2246,9 +2246,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
 	if (tbl_name != NULL && space_is_system(space)) {
 		diag_set(ClientError, ER_MODIFY_INDEX, name, def->name,
 			 "can't create index on system space");
-		parse->nErr++;
-		parse->rc = SQL_TARANTOOL_ERROR;
-		goto exit_create_index;
+		goto tnt_error;
 	}
 
 	/*
@@ -2275,9 +2273,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
 	index = (struct index *) region_alloc(&parse->region, sizeof(*index));
 	if (index == NULL) {
 		diag_set(OutOfMemory, sizeof(*index), "region", "index");
-		parse->rc = SQL_TARANTOOL_ERROR;
-		parse->nErr++;
-		goto exit_create_index;
+		goto tnt_error;
 	}
 	memset(index, 0, sizeof(*index));
 
@@ -2436,6 +2432,10 @@ sql_create_index(struct Parse *parse, struct Token *token,
 	sql_expr_list_delete(db, col_list);
 	sqlSrcListDelete(db, tbl_name);
 	sqlDbFree(db, name);
+	return;
+tnt_error:
+	sql_parser_error(parse);
+	goto exit_create_index;
 }
 
 void
@@ -2447,32 +2447,28 @@ sql_drop_index(struct Parse *parse_context, struct SrcList *index_name_list,
 	struct sql *db = parse_context->db;
 	/* Never called with prior errors. */
 	assert(parse_context->nErr == 0);
-	assert(table_token != NULL);
-	const char *table_name = sqlNameFromToken(db, table_token);
-	if (db->mallocFailed) {
-		goto exit_drop_index;
-	}
+	const char *table_name = sql_name_from_token(db, table_token);
+	if (table_name == NULL)
+		goto tnt_error;
 	sqlVdbeCountChanges(v);
 	assert(index_name_list->nSrc == 1);
 	assert(table_token->n > 0);
 	struct space *space = space_by_name(table_name);
 	if (space == NULL) {
-		if (!if_exists) {
-			diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
-			sql_parser_error(parse_context);
-		}
-		goto exit_drop_index;
+		if (if_exists)
+			goto exit_drop_index;
+		diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
+		goto tnt_error;
 	}
 	const char *index_name = index_name_list->a[0].zName;
 	uint32_t index_id = box_index_id_by_name(space->def->id, index_name,
 						 strlen(index_name));
 	if (index_id == BOX_ID_NIL) {
-		if (!if_exists) {
-			diag_set(ClientError, ER_NO_SUCH_INDEX_NAME,
-				 index_name, table_name);
-			sql_parser_error(parse_context);
-		}
-		goto exit_drop_index;
+		if (if_exists)
+			goto exit_drop_index;
+		diag_set(ClientError, ER_NO_SUCH_INDEX_NAME, index_name,
+			 table_name);
+		goto tnt_error;
 	}
 	struct index *index = space_index(space, index_id);
 	assert(index != NULL);
@@ -2493,6 +2489,10 @@ sql_drop_index(struct Parse *parse_context, struct SrcList *index_name_list,
  exit_drop_index:
 	sqlSrcListDelete(db, index_name_list);
 	sqlDbFree(db, (void *) table_name);
+	return;
+tnt_error:
+	sql_parser_error(parse_context);
+	goto exit_drop_index;
 }
 
 /*
@@ -2553,7 +2553,7 @@ sql_id_list_append(struct sql *db, struct IdList *list,
 				   &list->nId, &i);
 	if (i < 0)
 		goto error;
-	list->a[i].zName = sqlNameFromToken(db, name_token);
+	list->a[i].zName = sql_name_from_token(db, name_token);
 	if (list->a[i].zName == NULL)
 		goto error;
 	return list;
@@ -2668,7 +2668,13 @@ sql_src_list_append(struct sql *db, struct SrcList *list,
 		list = new_list;
 	}
 	struct SrcList_item *item = &list->a[list->nSrc - 1];
-	item->zName = sqlNameFromToken(db, name_token);
+	if (name_token != NULL) {
+		item->zName = sql_name_from_token(db, name_token);
+		if (item->zName == NULL) {
+			sqlSrcListDelete(db, list);
+			return NULL;
+		}
+	}
 	return list;
 }
 
@@ -2769,8 +2775,12 @@ sqlSrcListAppendFromTerm(Parse * pParse,	/* Parsing context */
 	assert(p->nSrc != 0);
 	pItem = &p->a[p->nSrc - 1];
 	assert(pAlias != 0);
-	if (pAlias->n) {
-		pItem->zAlias = sqlNameFromToken(db, pAlias);
+	if (pAlias->n != 0) {
+		pItem->zAlias = sql_name_from_token(db, pAlias);
+		if (pItem->zAlias == NULL) {
+			sql_parser_error(pParse);
+			goto append_from_error;
+		}
 	}
 	pItem->pSelect = pSubquery;
 	pItem->pOn = pOn;
@@ -2805,8 +2815,15 @@ sqlSrcListIndexedBy(Parse * pParse, SrcList * p, Token * pIndexedBy)
 			 */
 			pItem->fg.notIndexed = 1;
 		} else {
-			pItem->u1.zIndexedBy =
-			    sqlNameFromToken(pParse->db, pIndexedBy);
+			if (pIndexedBy->z != NULL) {
+				pItem->u1.zIndexedBy =
+					sql_name_from_token(pParse->db,
+							    pIndexedBy);
+				if (pItem->u1.zIndexedBy == NULL) {
+					sql_parser_error(pParse);
+					return;
+				}
+			}
 			pItem->fg.isIndexedBy = (pItem->u1.zIndexedBy != 0);
 		}
 	}
@@ -2892,11 +2909,12 @@ sql_transaction_rollback(Parse *pParse)
 void
 sqlSavepoint(Parse * pParse, int op, Token * pName)
 {
-	char *zName = sqlNameFromToken(pParse->db, pName);
+	struct sql *db = pParse->db;
+	char *zName = sql_name_from_token(db, pName);
 	if (zName) {
 		Vdbe *v = sqlGetVdbe(pParse);
 		if (!v) {
-			sqlDbFree(pParse->db, zName);
+			sqlDbFree(db, zName);
 			return;
 		}
 		if (op == SAVEPOINT_BEGIN &&
@@ -2906,6 +2924,8 @@ sqlSavepoint(Parse * pParse, int op, Token * pName)
 			return;
 		}
 		sqlVdbeAddOp4(v, OP_Savepoint, op, 0, 0, zName, P4_DYNAMIC);
+	} else {
+		sql_parser_error(pParse);
 	}
 }
 
@@ -2981,19 +3001,23 @@ sqlWithAdd(Parse * pParse,	/* Parsing context */
 {
 	sql *db = pParse->db;
 	With *pNew;
-	char *zName;
 
-	/* Check that the CTE name is unique within this WITH clause. If
-	 * not, store an error in the Parse structure.
+	/*
+	 * Check that the CTE name is unique within this WITH
+	 * clause. If not, store an error in the Parse structure.
 	 */
-	zName = sqlNameFromToken(pParse->db, pName);
-	if (zName && pWith) {
+	char *name = sql_name_from_token(db, pName);
+	if (name == NULL) {
+		sql_parser_error(pParse);
+		return NULL;
+	}
+	if (pWith != NULL) {
 		int i;
 		for (i = 0; i < pWith->nCte; i++) {
-			if (strcmp(zName, pWith->a[i].zName) == 0) {
+			if (strcmp(name, pWith->a[i].zName) == 0) {
 				sqlErrorMsg(pParse,
-						"duplicate WITH table name: %s",
-						zName);
+					    "duplicate WITH table name: %s",
+					    name);
 			}
 		}
 	}
@@ -3005,17 +3029,17 @@ sqlWithAdd(Parse * pParse,	/* Parsing context */
 	} else {
 		pNew = sqlDbMallocZero(db, sizeof(*pWith));
 	}
-	assert((pNew != 0 && zName != 0) || db->mallocFailed);
+	assert((pNew != NULL && name != NULL) || db->mallocFailed);
 
 	if (db->mallocFailed) {
 		sql_expr_list_delete(db, pArglist);
 		sql_select_delete(db, pQuery);
-		sqlDbFree(db, zName);
+		sqlDbFree(db, name);
 		pNew = pWith;
 	} else {
 		pNew->a[pNew->nCte].pSelect = pQuery;
 		pNew->a[pNew->nCte].pCols = pArglist;
-		pNew->a[pNew->nCte].zName = zName;
+		pNew->a[pNew->nCte].zName = name;
 		pNew->a[pNew->nCte].zCteErr = 0;
 		pNew->nCte++;
 	}
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 8b909f2e1..97b716d47 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -423,19 +423,25 @@ sqlPragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
 	sqlVdbeRunOnlyOnce(v);
 	pParse->nMem = 2;
 
-	zLeft = sqlNameFromToken(db, pId);
-	if (!zLeft) {
+	if (pId == NULL) {
 		vdbe_emit_pragma_status(pParse);
 		return;
 	}
-
+	zLeft = sql_name_from_token(db, pId);
+	if (zLeft == NULL)
+		goto tnt_error;
 	if (minusFlag) {
 		zRight = sqlMPrintf(db, "-%T", pValue);
-	} else {
-		zRight = sqlNameFromToken(db, pValue);
+	} else if (pValue != NULL) {
+		zRight = sql_name_from_token(db, pValue);
+		if (zRight == NULL)
+			goto tnt_error;
+	}
+	if (pValue2 != NULL) {
+		zTable = sql_name_from_token(db, pValue2);
+		if (zTable == NULL)
+			goto tnt_error;
 	}
-	zTable = sqlNameFromToken(db, pValue2);
-
 	/* Locate the pragma in the lookup table */
 	pPragma = pragmaLocate(zLeft);
 	if (pPragma == 0) {
@@ -615,4 +621,8 @@ sqlPragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
 	sqlDbFree(db, zLeft);
 	sqlDbFree(db, zRight);
 	sqlDbFree(db, zTable);
+	return;
+tnt_error:
+	sql_parser_error(pParse);
+	goto pragma_out;
 }
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 56ae90a95..b49229b26 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3644,7 +3644,26 @@ int sqlExprCodeExprList(Parse *, ExprList *, int, int, u8);
 void sqlExprIfTrue(Parse *, Expr *, int, int);
 void sqlExprIfFalse(Parse *, Expr *, int, int);
 
-char *sqlNameFromToken(sql *, Token *);
+/**
+ * Given a token, return a string that consists of the text of
+ * that token. Space to hold the returned string is obtained
+ * from sqlMalloc() and must be freed by the calling function.
+ *
+ * Any quotation marks (ex:  "name", 'name', [name], or `name`)
+ * that surround the body of the token are removed.
+ *
+ * Tokens are often just pointers into the original SQL text and
+ * so are not \000 terminated and are not persistent. The returned
+ * string is \000 terminated and is persistent.
+ *
+ * @param db The database connection.
+ * @param name_token The source token with text.
+ * @retval Not NULL string pointer on success.
+ * @retval NULL Otherwise. Diag message is set.
+ */
+char *
+sql_name_from_token(struct sql *db, struct Token *name_token);
+
 int sqlExprCompare(Expr *, Expr *, int);
 int sqlExprListCompare(ExprList *, ExprList *, int);
 int sqlExprImpliesExpr(Expr *, Expr *, int);
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 20d41e6a1..65218121b 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -82,9 +82,9 @@ sql_trigger_begin(struct Parse *parse, struct Token *name, int tr_tm,
 		goto trigger_cleanup;
 	assert(table->nSrc == 1);
 
-	trigger_name = sqlNameFromToken(db, name);
+	trigger_name = sql_name_from_token(db, name);
 	if (trigger_name == NULL)
-		goto trigger_cleanup;
+		goto set_tarantool_error_and_cleanup;
 
 	if (sqlCheckIdentifierName(parse, trigger_name) != SQL_OK)
 		goto trigger_cleanup;
-- 
2.21.0