[tarantool-patches] Re: [PATCH v2 7/7] sql: store regular identifiers in case-normal form

[Vladislav Shpilevoy]

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;