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

[v.shpilevoy at tarantool.org]

> 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?

> 
> 
>> 
>>> 		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)'.
> 
>> 
>>> 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.

> 
> -               for (i = 0; i < (int)index_column_count(pPk); i++) {
> +               int col_count = index_column_count(pPk);
> +               for (i = 0; i < col_count; i++) {
> 
>