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

v.shpilevoy at tarantool.org v.shpilevoy at tarantool.org
Wed Mar 7 13:19:18 MSK 2018


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

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

> +	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.

> +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.

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




More information about the Tarantool-patches mailing list