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 663E428355 for ; Mon, 18 Mar 2019 15:33:57 -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 xjrqdD7TXk45 for ; Mon, 18 Mar 2019 15:33:57 -0400 (EDT) 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 166C527BB3 for ; Mon, 18 Mar 2019 15:33:57 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 7/7] sql: store regular identifiers in case-normal form References: <6f979039-61fc-1ee7-e006-859e85195b33@tarantool.org> <152eb589-a416-01d4-e75c-8876c9538c9d@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Mon, 18 Mar 2019 22:33:54 +0300 MIME-Version: 1.0 In-Reply-To: <152eb589-a416-01d4-e75c-8876c9538c9d@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 Thanks for the fixes! See 3 comments below. On 11/03/2019 18:04, Kirill Shcherbatov wrote: >> First of all, I do not see a test on failed normalization. >> Please, add. > I've implemented a new errinj test. > >> 1. You have sql_parser_error, and you used it already in >> some other places. Fix it, please, in all touched places. > > Ok, done. > >> 2. Why? > > Because I've added a new test that have created a new additional table. 1. As I asked multiple times - please, do not put my comments out of the context. I see my question 'Why?', but I do not see what does it refer to. > diff --git a/src/box/sql/util.c b/src/box/sql/util.c > index c89e2e8ab..60133758a 100644 > --- a/src/box/sql/util.c > +++ b/src/box/sql/util.c > @@ -292,23 +294,37 @@ sqlDequote(char *z) > z[j] = 0; > } > > - > -void > -sqlNormalizeName(char *z) > +int > +sql_normalize_name(char *dst, int dst_size, const char *src, int src_len) > { > - char quote; > - int i=0; > - if (z == 0) > - return; > - quote = z[0]; > - if (sqlIsquote(quote)){ > - sqlDequote(z); > - return; > - } > - while(z[i]!=0){ > - z[i] = (char)sqlToupper(z[i]); > - i++; > + assert(src != NULL); > + if (sqlIsquote(src[0])){ > + if (dst_size == 0) > + return src_len; > + memcpy(dst, src, src_len); > + dst[src_len] = '\0'; > + sqlDequote(dst); > + return src_len; 2. In the comment you said that sql_normalize_name returns a length of result string, but here it is false. sqlDequote can trim some symbols and the result length is less. The code works only thanks to the fact, that the result of sql_normalize_name is never used as a length, but rather as a buffer size. Please, either fix the return value, returning a real string length, or change the comment, saying what that function returns + its usage places, where you store its result into 'name_len' variables (they should be renamed to bsize or something). > diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua > index c876f9afb..9e8095295 100644 > --- a/test/box/errinj.test.lua > +++ b/test/box/errinj.test.lua > @@ -587,3 +587,11 @@ fio = require('fio') > #fio.glob(fio.pathjoin(box.cfg.vinyl_dir, box.space.test.id, 0, '*.index.inprogress')) == 0 > > box.space.test:drop() > + > +-- > +-- gh-3931: Store regular identifiers in case-normal form > +-- > +errinj = box.error.injection > +errinj.set("ERRINJ_SQL_NAME_NORMALIZATION", true) > +box.sql.execute("CREATE TABLE hello (id INT primary key,x INT,y INT);") > +errinj.set("ERRINJ_SQL_NAME_NORMALIZATION", false) 3. We have sql/errinj.test.lua for that. Below you can find a couple of general comments, which I discovered on the top commit, but probably some of them were introduced in the previous ones: 1) delete.c:72: 'where' leaks. It was duplicated on line 68, and you do not free it before return; 2) fk_constraint.c:641: why not to return? Here OOM has happened, obviously, and there is nothing to cleanup;