[tarantool-patches] Re: [PATCH 2/2] sql: replace KeyInfo with key_def
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri May 11 14:22:44 MSK 2018
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.
More information about the Tarantool-patches
mailing list