[patches] [PATCH] sql: remove unused fields from SQL index

[n.pettik]

> On 7 Mar 2018, at 18:27, v.shpilevoy at tarantool.org wrote:
> 
> 
> 
>> 7 марта 2018 г., в 17:33, n.pettik <korablev at tarantool.org> написал(а):
>> 
>> 
>>> On 7 Mar 2018, at 13:19, v.shpilevoy at tarantool.org wrote:
>>> 
>>> See below 6 comments.
>>> 
>>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>>>> index 9f45c6224..f2011163c 100644
>>>> --- a/src/box/sql/build.c
>>>> +++ b/src/box/sql/build.c
>>>> @@ -3375,14 +3367,53 @@ sqlite3DefaultRowEst(Index * pIdx)
>>>> 	 * 6 and each subsequent value (if any) is 5.
>>>> 	 */
>>>> 	memcpy(&a[1], aVal, nCopy * sizeof(LogEst));
>>>> -	for (i = nCopy + 1; i <= pIdx->nKeyCol; i++) {
>>>> +	for (i = nCopy + 1; i <= pIdx->nColumn; i++) {
>>> 
>>> 1. Why do you use here pIdx->nColumn, but in other places index_column_count()?
>> 
>> As far as I could remember, my initial intend lies in the fact that this function is called
>> only inside build routine (i.e. before space is actually created), so there is no
>> reason at attempting to fetch column number from Tarantool.
> 
> If it must be called only inside build, then why it is called in sqlite3DeleteFrom, sqlite3GenerateRowIndexDelete, sqlite3Pragma, xferCompatibleIndex and other non-build functions?

You misunderstood me. I mean sqlite3DefaultRowEst() is called before space construction, not index_column_count().
And before space construction there is no reason to call index_column_count(), since
it won’t find space in space cache and will return column count from SQL index.
(But now I grep it again and it turns out that sqlite3DefaultRowEst() is also used in analyze routine, so
 basically it may be called after space creation. Anyway, I have already fixed it.)

> 
>> 
>> 
>>> 
>>>> 		a[i] = 23;
>>>> 		assert(23 == sqlite3LogEst(5));
>>>> 	}
>>>> 
>>>> 	assert(0 == sqlite3LogEst(1));
>>>> 	if (IsUniqueIndex(pIdx))
>>>> -		a[pIdx->nKeyCol] = 0;
>>>> +		a[pIdx->nColumn] = 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * Return number of columns in given index.
>>>> + * If space is ephemeral or represents view, use internal
>>>> + * SQL structure to fetch the value.
>>>> + */
>>>> +inline uint32_t
>>>> +index_column_count(Index *idx)
>>>> +{
>>>> +	assert(idx != NULL);
>>>> +	uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(idx->tnum);
>>>> +	struct space *space = space_by_id(space_id);
>>>> +	/* It is impossible to find an ephemeral space by id. */
>>>> +	if (space == NULL)
>>>> +		return idx->nColumn;
>>>> +
>>>> +	uint32_t index_id = SQLITE_PAGENO_TO_INDEXID(idx->tnum);
>>>> +	struct index *index = space_index(space, index_id);
>>>> +	/* Views don't feature any functional parts. */
>>>> +	if (index == NULL || index->def->key_def->part_count == 0)
>>>> +		return idx->nColumn;
>>> 
>>> 2. View space can not be found using space_by_id, so this code is unreachable, it is not?
>> 
>> Indeed, but this function would never be called for ‘pure’ views
>> (they don’t have any indexes to be called with this function).
> 
> If so, then why do you not delete this check on a branch?
> 
>> In SQLite views are materialized into ephemeral spaces, which in their turn already have indexes.
> 
> Ephemeral space is not a space cache, and its index_column_count() exits on 'if (space == NULL)’.

I’m sorry, it is my bad. It is definitely redundant if-check. Replaced it with assert on branch:

-       /* Views don't feature any functional parts. */
-       if (index == NULL || index->def->key_def->part_count == 0)
-               return idx->nColumn;
+       assert(index != NULL);

>> 
>>> 
>>>> diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
>>>> index 088d84e27..c7917c595 100644
>>>> --- a/src/box/sql/fkey.c
>>>> +++ b/src/box/sql/fkey.c
>>>> @@ -668,7 +669,7 @@ fkScanChildren(Parse * pParse,	/* Parse context */
>>>> 		Expr *pEq, *pAll = 0;
>>>> 		Index *pPk = sqlite3PrimaryKeyIndex(pTab);
>>>> 		assert(pIdx != 0);
>>>> -		for (i = 0; i < pPk->nKeyCol; i++) {
>>>> +		for (i = 0; i < (int)index_column_count(pPk); i++) {
>>> 
>>> 5. If you write a function in for condition, is it called on each iteration with -O3 compiler flag? I do not know and on just in case I would save a result of index_column_count() in a variable and use it in the for condition, like it is done below.
>> 
>> I guess, compiler is smart enough for such optimisation. However, fixed anyway to avoid any suspicions:
> 
> To help compiler make index_column_count parameter be const struct Index * instead of struct Index *, please.

Done.

Should I send second version of patch, or these changes are considered to be minor?