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 4B17F2939D for ; Thu, 7 Mar 2019 12:34:27 -0500 (EST) 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 Dd_MUsBhghlI for ; Thu, 7 Mar 2019 12:34:27 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 03C1928BEA for ; Thu, 7 Mar 2019 12:34:26 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v2 3/7] sql: refactor sql_src_list_append to set diag References: <390fc2f46ba80994aa4ebf97d3aeb8ea8c26c71c.1551265819.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <4dcd389e-2388-d145-a82c-2cabc2b9ed15@tarantool.org> Date: Thu, 7 Mar 2019 20:34:24 +0300 MIME-Version: 1.0 In-Reply-To: <390fc2f46ba80994aa4ebf97d3aeb8ea8c26c71c.1551265819.git.kshcherbatov@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed 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, Kirill Shcherbatov See 4 comments below. On 27/02/2019 14:13, Kirill Shcherbatov wrote: > 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; 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. > + list = new_list; > + } > + pItem = &list->a[list->nSrc - 1]; > + pItem->zName = sqlNameFromToken(db, name_token); 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. > + 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; 3. The same as 1. > + assert(p->nSrc != 0); > pItem = &p->a[p->nSrc - 1]; > assert(pAlias != 0); > if (pAlias->n) { > 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. 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. > + */ > +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 *);