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 6168528BE6 for ; Thu, 7 Mar 2019 12:34:21 -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 6LFjcLVQxnDE for ; Thu, 7 Mar 2019 12:34:21 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 C165C28912 for ; Thu, 7 Mar 2019 12:34:20 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v2 1/7] sql: refactor sql_alloc_src_list to set diag References: <9bc544e742ad88f4fd6a3738fb4993d789832bc6.1551265819.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <1aa67789-567f-4e38-0d7d-db45f302c6d4@tarantool.org> Date: Thu, 7 Mar 2019 20:34:18 +0300 MIME-Version: 1.0 In-Reply-To: <9bc544e742ad88f4fd6a3738fb4993d789832bc6.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 Hi! Thanks for the patch! On 27/02/2019 14:13, Kirill Shcherbatov wrote: > Refactored sql_alloc_src_list routine to use diag_set in case of > memory allocation error. This will ensure that the > sqlSrcListAppend function throws an error using diag in > subsequent patches. The issue was about normalization error, but here I do not see a word about it. Only an obfuscated necessity to refactor sqlSrcListAppend. Why do you needed it? How it makes you closer to the name normalization? I spent significant time to recover the whole tree of functions, using name normalization, in order to learn what sqlSrcListAppend has to do with it. Please, write more accurate and elaborated comments to each commit in the patchset. Also, as I see, you decided to go from the bottom and firstly refactor some helper functions, after which you fixed the functions who use normalization. But the thing is that this commit and some of the others do not work because you set diag, but diag error never reaches a user. 1) In this commit you refactored sql_alloc_str_list, its usage in sqlSrcListAppend remained to be non-tarantool. It means, that if sql_alloc_str_list fails in sqlSrcListAppend and sets a diag, it is never thrown to a user. 2) About commit 'sql: rework sql_src_list_enlarge to set diag': if sql_src_list_enlarge fails in sqlSrcListAppend or in src_list_append_unique, the diag will never be thrown. 3) About commit 'sql: refactor sql_name_from_token to set diag': if sql_name_from_token fails in sql_id_list_append or sqlSrcListIndexedBy - the diag error is lost. I do not see a way how to gracefully split the patch into multiple preparation patches, and keep diag working, so I ask you to just write a comment in the commit messages, listed above, that they are not self-sufficient. > > Needed for #3931 > --- > src/box/sql/build.c | 30 +++++++++++++++++------------- > src/box/sql/sqlInt.h | 12 ++++++++++-- > 2 files changed, 27 insertions(+), 15 deletions(-) > > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index dbfdbc6f5..ce29dc0b1 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -3427,8 +3427,16 @@ void *sqlArrayAllocate(sql *, void *, int, int *, int *); > IdList *sqlIdListAppend(sql *, IdList *, Token *); > int sqlIdListIndex(IdList *, const char *); > SrcList *sqlSrcListEnlarge(sql *, SrcList *, int, int); > -SrcList * > -sql_alloc_src_list(sql *db); > + > +/** > + * Allocate a new empty SrcList object. > + * @param db The database connection. > + * @retval not NULL list pointer on success. > + * @retval NULL otherwise. As I've already said - please, use capital letters at the beginning of a sentence. Also, it is worth mentioning that it sets diag, and db->mallocFailed means nothing. The same about all other refactored functions in scope of this patchset. > + */ > +struct SrcList * > +sql_alloc_src_list(struct sql *db); > + > SrcList *sqlSrcListAppend(sql *, SrcList *, Token *); > SrcList *sqlSrcListAppendFromTerm(Parse *, SrcList *, Token *, > Token *, Select *, Expr *, IdList *); > -- > 2.20.1 > >