[tarantool-patches] Re: [PATCH 2/2] sql: replace KeyInfo with key_def

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hello. Thanks for fixes. See travis: https://travis-ci.org/tarantool/tarantool/jobs/377267310.

See 10 comments below.

> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 029c71e..bfaf3af 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1119,10 +1131,9 @@ sql_index_collation(Index *idx, uint32_t column)
>   		return idx->coll_array[column];
>   	}
>   
> -	uint32_t index_id = SQLITE_PAGENO_TO_INDEXID(idx->tnum);
> -	struct index *index = space_index(space, index_id);
> -	assert(index != NULL && index->def->key_def->part_count >= column);
> -	return index->def->key_def->parts[column].coll;
> +	struct key_def *key_def = sql_index_key_def(idx);

1. Sql_index_key_def makes dup, that is not needed to get the collation.

2. It leaks.

> +	assert(key_def != NULL && key_def->part_count >= column);

3. Assertion can fail on OOM.
> @@ -1143,10 +1154,9 @@ sql_index_column_sort_order(Index *idx, uint32_t column)
>   		return idx->sort_order[column];
>   	}
>   
> -	uint32_t index_id = SQLITE_PAGENO_TO_INDEXID(idx->tnum);
> -	struct index *index = space_index(space, index_id);
> -	assert(index != NULL && index->def->key_def->part_count >= column);
> -	return index->def->key_def->parts[column].sort_order;
> +	struct key_def *key_def = sql_index_key_def(idx);
> +	assert(key_def != NULL && key_def->part_count >= column);
> +	return key_def->parts[column].sort_order;

4. All the same. Maybe it is better to do not dup key_def in sql_index_key_def
and just return it as is. And do dup() in caller code.


> @@ -2643,14 +2650,13 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
>   	} else {
>   		tnum = pIndex->tnum;
>   	}
> -	pKey = sqlite3KeyInfoOfIndex(pParse, db, pIndex);
> -	assert(pKey != 0 || db->mallocFailed || pParse->nErr);
> +	struct key_def *def = sql_index_key_def(pIndex);
> +	assert(def != NULL || db->mallocFailed || pParse->nErr);

5. Assertion can fail - when sql_index_key_def fails to do dup(),
it does not set mallocFailed and nErr.
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index 3f74b93..b6fc135 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -386,10 +386,10 @@ sqlite3DeleteFrom(Parse * pParse,	/* The parser context */
>   			iPk = pParse->nMem + 1;
>   			pParse->nMem += nPk;
>   			iEphCur = pParse->nTab++;
> -			KeyInfo *pKeyInfo = sqlite3KeyInfoAlloc(pParse->db, nPk, 0);
> +			struct key_def *def = key_def_new(nPk);

6. Nobody sets pParse->rc. Maybe it is better to declare a function like
parser_key_def_new, that takes all the same as key_def + struct Parse, and sets
rc = SQL_TARANTOOL_ERROR to get OOM from diag.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index cc969ca..635357b 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -1505,7 +1505,7 @@ sqlite3ExprListDup(sqlite3 * db, ExprList * p, int flags)
>   		}
>   		pItem->zName = sqlite3DbStrDup(db, pOldItem->zName);
>   		pItem->zSpan = sqlite3DbStrDup(db, pOldItem->zSpan);
> -		pItem->sortOrder = pOldItem->sortOrder;
> +		pItem->sort_order = pOldItem->sort_order;
>   		pItem->done = 0;
>   		pItem->bSpanIsTab = pOldItem->bSpanIsTab;
>   		pItem->u = pOldItem->u;
> @@ -1766,10 +1766,13 @@ sqlite3ExprListSetSortOrder(ExprList * p, int iSortOrder)
>   		return;
>   	assert(p->nExpr > 0);
>   	if (iSortOrder == SORT_ORDER_UNDEF) {
> -		assert(p->a[p->nExpr - 1].sortOrder == SORT_ORDER_ASC);
> +		assert(p->a[p->nExpr - 1].sort_order == SORT_ORDER_ASC);
>   		return;
>   	}
> -	p->a[p->nExpr - 1].sortOrder = (u8) iSortOrder;
> +	if (iSortOrder == 0)
> +		p->a[p->nExpr - 1].sort_order = SORT_ORDER_ASC;
> +	else
> +		p->a[p->nExpr - 1].sort_order = SORT_ORDER_DESC;

7. Lets make iSortOrder be enum sort_order and remove this 'if's.

> @@ -2761,7 +2763,7 @@ sqlite3CodeSubselect(Parse * pParse,	/* Parsing context */
>   			pExpr->is_ephemeral = 1;
>   			addr = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral,
>   						 pExpr->iTable, nVal);
> -			pKeyInfo = sqlite3KeyInfoAlloc(pParse->db, nVal, 1);
> +			struct key_def *key_def = key_def_new(nVal);

8. pParse->rc is not set to error.
> @@ -2787,29 +2789,34 @@ sqlite3CodeSubselect(Parse * pParse,	/* Parsing context */
>   					pSelect->iLimit = 0;
>   					testcase(pSelect->
>   						 selFlags & SF_Distinct);
> -					testcase(pKeyInfo == 0);	/* Caused by OOM in sqlite3KeyInfoAlloc() */
>   					if (sqlite3Select
>   					    (pParse, pSelect, &dest)) {
>   						sqlite3DbFree(pParse->db,
>   							      dest.zAffSdst);
> -						sqlite3KeyInfoUnref(pKeyInfo);
> +						if (key_def != NULL)
> +							free(key_def);
>   						return 0;
>   					}
>   					sqlite3DbFree(pParse->db,
>   						      dest.zAffSdst);
> -					assert(pKeyInfo != 0);	/* OOM will cause exit after sqlite3Select() */
> +					assert(key_def != NULL);

9. Assertion fails, if key_def == NULL. Maybe it is better to check key_def == NULL once right
after key_def_new, return on error, and remove this 'if (key_def != NULL)' everywhere.
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 1a34f71..95a2610 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -565,9 +565,9 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
>   			regRec = sqlite3GetTempReg(pParse);
>   			regCopy = sqlite3GetTempRange(pParse, nColumn);
>   			regTempId = sqlite3GetTempReg(pParse);
> -			KeyInfo *pKeyInfo = sqlite3KeyInfoAlloc(pParse->db, 1+nColumn, 0);
> +			struct key_def *def = key_def_new(nColumn + 1);

10. No error code is set. I will not repeat this comment below, but the error code is not
set in more places.