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 7544028F8C for ; Mon, 11 Mar 2019 11:04:52 -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 j64YT6RjQgfD for ; Mon, 11 Mar 2019 11:04:52 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 0C76528C01 for ; Mon, 11 Mar 2019 11:04:52 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 3/7] sql: refactor sql_src_list_append to set diag References: <390fc2f46ba80994aa4ebf97d3aeb8ea8c26c71c.1551265819.git.kshcherbatov@tarantool.org> <4dcd389e-2388-d145-a82c-2cabc2b9ed15@tarantool.org> From: Kirill Shcherbatov Message-ID: <8743fe4b-6485-9b72-dd40-e4d2ecab5801@tarantool.org> Date: Mon, 11 Mar 2019 18:04:50 +0300 MIME-Version: 1.0 In-Reply-To: <4dcd389e-2388-d145-a82c-2cabc2b9ed15@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 > 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