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

[n.pettik]

> 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.
But to unify code, I will replace it with function call. Fixed on branch:

-       for (i = nCopy + 1; i <= pIdx->nColumn; i++) {
+       int col_count = index_column_count(pIdx);
+       for (i = nCopy + 1; i <= col_count; i++) {

> 
>> 		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).
In SQLite views are materialized into ephemeral spaces, which in their turn already have indexes.

> 
>> +	return index->def->key_def->part_count;
>> +}
>> +
>> +/* Return true if given index is unique and not nullable. */
> 
> 3. For function description at the beginning use /** instead of /*, please.

Fixed on branch:

-/* Return true if given index is unique and not nullable. */
-inline         bool
+/** Return true if given index is unique and not nullable. */
+bool

>> +inline 	bool
> 
> 4. At first, this function is declared in a header as non-static and non-inline, so it is not inlined. At second, please, write one whitespace between function return type and attributes like inline, static.

We have got complete mess with headers in SQLite (two big headers which contain ).
Thus, it is not quite easy to put inline function to one of them, if it uses definition from both (cyclic dependencies).
Overall, I just removed this inline modifier. Anyway, I suppose compiler will cope with inlining itself.

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

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

>> 
>> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
>> index 2f1c627e5..8d1e3a393 100644
>> --- a/src/box/sql/where.c
>> +++ b/src/box/sql/where.c
>> @@ -557,7 +557,7 @@ isDistinctRedundant(Parse * pParse,		/* Parsing context */
>> 	for (pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext) {
>> 		if (!IsUniqueIndex(pIdx))
>> 			continue;
>> -		for (i = 0; i < pIdx->nKeyCol; i++) {
>> +		for (i = 0; i < (int)index_column_count(pIdx); i++) {
> 
> 6. Same as 5.

The same (fixed on branch):

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

Also, fixed build error from Travis:
src/box/sql/where.c
@@ -2593,8 +2596,11 @@ whereLoopAddBtreeIndex(WhereLoopBuilder * pBuilder,	/* The WhereLoop factory */

-		if (saved_nEq == saved_nSkip && saved_nEq + 1 < nProbeCol &&
+		if (saved_nEq == saved_nSkip && saved_nEq + 1U < nProbeCol &&