[tarantool-patches] Re: [PATCH v2 1/1] sql: do not analyze incorrect statistics

[Vladislav Shpilevoy]

Hi! Thanks for the patch! See 1 comment below.

On 26/12/2018 22:01, imeevma at tarantool.org wrote:
> Vlad, please, do second review. I rebased it to newest 2.1.
> 
> https://github.com/tarantool/tarantool/issues/3866
> https://github.com/tarantool/tarantool/tree/imeevma/gh-3866-ignore-wrong-data-in-_sql_stat
> 
> 
> On 12/24/18 5:15 PM, n.pettik wrote:
>>
>>> New version:
>>>
>>> commit 1b43714d03ae2a2f7042415aed3d60e1a41034f0
>>> Author: Mergen Imeev <imeevma at gmail.com>
>>> Date:   Wed Dec 19 21:12:17 2018 +0300
>>>
>> Now it looks pretty good. LGTM.
>>
> 
> commit 9a564cdf194685401f6b57d69408282d6326b856
> Author: Mergen Imeev <imeevma at gmail.com>
> Date:   Wed Dec 19 21:12:17 2018 +0300
> 
>      sql: do not analyze incorrect statistics
>      
>      Some errors that occurred during the analysis were processed
>      without an error message. However, these errors should not be
>      processed, as they show that something is wrong with the data
>      received. After this patch, entries in _sql_stat* with the wrong
>      space or index name will be ignored.
>      
>      Closes #3866
> 
> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index 51c63fa..d1fa4ec 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c

Analyze.c actively uses decode_stat_string() which assumes that
each stat record contains a valid list of white-space separated
unsigned integers. But what if I insert a non-integer? A char? A
negative integer? They are not ignored, but weirdly interpreted. I
think, such rows should not be accounted at all. Just like when a
space/index are not found.

The patch is OK except this thing.

> @@ -1224,10 +1224,10 @@ analysis_loader(void *data, int argc, char **argv, char **unused)
>   		return 0;
>   	struct analysis_index_info *info = (struct analysis_index_info *) data;
>   	assert(info->stats != NULL);
> -	struct index_stat *stat = &info->stats[info->index_count++];
> +	struct index_stat *stat = &info->stats[info->index_count];
>   	struct space *space = space_by_name(argv[0]);
>   	if (space == NULL)
> -		return -1;
> +		return 0;
>   	struct index *index;
>   	uint32_t iid = box_index_id_by_name(space->def->id, argv[1],
>   					    strlen(argv[1]));