[tarantool-patches] Re: [PATCH 4/4] sql: move SQL statistics to server

[n.pettik]

>> +		dup->samples[i].dlt = (uint32_t *)((char*)dup +
>> +						   current_sample_offset +
>> +						   2 * size);
> 
> 1. Typo? Maybe array_size instead of size? I slightly refactored it. Take a look. Looks
> more compact, and tests are passed.

Yep, it was definitely typo. Anyway, lets use your variant since it seems to be
easier to understand.

> I wish it was my task, huge malloc formatting is very interesting.

I wish I could understand whether this is sarcasm or not...

> 
> diff --git a/src/box/index_def.c b/src/box/index_def.c
> index 1702430b3..d9035e3c5 100644
> --- a/src/box/index_def.c
> +++ b/src/box/index_def.c
> @@ -205,31 +205,25 @@ index_stat_dup(const struct index_stat *src)
> 	memcpy(dup, src, size);
> 	uint32_t array_size = src->sample_field_count * sizeof(uint32_t);
> 	uint32_t stat1_offset = sizeof(struct index_stat);
> -	dup->tuple_stat1 = (uint32_t *)((char*)dup + stat1_offset);
> -	dup->tuple_log_est = (log_est_t *)((char*)dup + stat1_offset +
> -					   array_size + sizeof(uint32_t));
> -	dup->avg_eq = (uint32_t *)((char*)dup + stat1_offset +
> -				   2 * (array_size + sizeof(uint32_t)));
> -	uint32_t samples_offset = stat1_offset + 3 * array_size +
> -				  2 * sizeof(uint32_t);
> -	dup->samples = (struct index_sample *)((char*)dup + samples_offset);
> -	uint32_t current_sample_offset = samples_offset +
> -					 src->sample_count *
> -					 sizeof(struct index_sample);
> +	char *pos = (char *) dup + stat1_offset;
> +	dup->tuple_stat1 = (uint32_t *) pos;
> +	pos += array_size + sizeof(uint32_t);
> +	dup->tuple_log_est = (log_est_t *) pos;
> +	pos += array_size + sizeof(uint32_t);
> +	dup->avg_eq = (uint32_t *) pos;
> +	pos += array_size;
> +	dup->samples = (struct index_sample *) pos;
> +	pos += src->sample_count * sizeof(struct index_sample);
> +
> 	for (uint32_t i = 0; i < src->sample_count; ++i) {
> -		dup->samples[i].eq = (uint32_t *)((char*)dup +
> -						  current_sample_offset);
> -		dup->samples[i].lt = (uint32_t *)((char*)dup +
> -						  current_sample_offset +
> -						  array_size);
> -		dup->samples[i].dlt = (uint32_t *)((char*)dup +
> -						   current_sample_offset +
> -						   2 * size);
> -		dup->samples[i].sample_key =
> -			(char *)((char*)dup + current_sample_offset + 3 * size);
> -		current_sample_offset += current_sample_offset +
> -					 3 * array_size +
> -					 dup->samples[i].key_size + 2;
> +		dup->samples[i].eq = (uint32_t *) pos;
> +		pos += array_size;
> +		dup->samples[i].lt = (uint32_t *) pos;
> +		pos += array_size;
> +		dup->samples[i].dlt = (uint32_t *) pos;
> +		pos += array_size;
> +		dup->samples[i].sample_key = pos;
> +		pos += dup->samples[i].key_size + 2;
> 	}
> 	return dup;
> }

Ok, your way rather better.

>> +/**
>> + * Duplicate index_stat object.
>> + * To understand memory layout see index_stat_sizeof() function.
>> + *
>> + * @param src Stat to duplicate.
>> + * @retval Copy of the @src.
> 
> 3. Or NULL.

Ok, added clarification.

>> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
>> index 57cac437c..4f21cd25f 100644
>> --- a/src/box/sql/analyze.c
>> +++ b/src/box/sql/analyze.c
>> +decode_stat_string(const char *stat_string, int stat_size, tRowcnt *stat_exact,
>> +		   LogEst *stat_log) {
>> +	char *z = (char *) stat_string;
> 
> 4. Why do you need this cast?

Since implicit cast discards const modifier and leads to compilation error.

> 
>> +		/*
>> +		 * Take a copy of the sample. Add two 0x00 bytes
>> +		 * the end of the buffer. This is in case the
>> +		 * sample record is corrupted. In that case, the
>> +		 * sqlite3VdbeRecordCompare() may read up to two
>> +		 * varints past the end of the allocated buffer
>> +		 * before it realizes it is dealing with a corrupt
>> +		 * record. Adding the two 0x00 bytes prevents this
>> +		 * from causing a buffer overread.
> 
> 5. Are you sure that this 0x0000 is still needed? Tarantool does not stores
> any "corrupted" things. Maybe it is Btree artifacts?
> 

Well, no corruptions - no zero bytes. I have removed mentions about
these bytes both from comments and code. Everything seems to be OK.
Also, functions which are supposed to rely on this condition (i.e. 0x00 bytes)
are unused. Thus, I will remove them in separate commit.

> 
> 6. How about this diff?

Pretty nice, I have updated my branch with your changes.