Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: tarantool-patches@freelists.org, Kirill Yukhin <kyukhin@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH 2/2] sql: replace KeyInfo with key_def
Date: Fri, 11 May 2018 14:22:44 +0300	[thread overview]
Message-ID: <af19ae87-e7af-c57d-8f3b-17c730325cad@tarantool.org> (raw)
In-Reply-To: <20180510125938.o6r3gxjfxmsusl5y@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.

  reply	other threads:[~2018-05-11 11:22 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-08  7:56 [tarantool-patches] [PATCH 0/2] sql: replace KeyInfo w/ key_def in SQL front-end Kirill Yukhin
2018-05-08  7:56 ` [tarantool-patches] [PATCH 1/2] sql: introduce sort order to key_part/key_part_def Kirill Yukhin
2018-05-08 16:02   ` [tarantool-patches] " Vladislav Shpilevoy
2018-05-10 13:01     ` Kirill Yukhin
2018-05-08  7:56 ` [tarantool-patches] [PATCH] sql: use collation pointers instead of names Kirill Yukhin
2018-04-17 18:06   ` [tarantool-patches] " Vladislav Shpilevoy
2018-04-18  5:42     ` Kirill Yukhin
2018-05-08  7:59   ` Kirill Yukhin
2018-05-08  7:56 ` [tarantool-patches] [PATCH 2/2] sql: replace KeyInfo with key_def Kirill Yukhin
2018-05-08 16:02   ` [tarantool-patches] " Vladislav Shpilevoy
2018-05-10 12:59     ` Kirill Yukhin
2018-05-11 11:22       ` Vladislav Shpilevoy [this message]
2018-05-11 12:56         ` Kirill Yukhin
2018-05-11 19:05           ` Vladislav Shpilevoy
2018-05-14 11:40             ` Kirill Yukhin
  -- strict thread matches above, loose matches on Subject: below --
2018-04-13  8:05 [tarantool-patches] [PATCH] sql: use collation pointers instead of names Kirill Yukhin
2018-04-16 13:43 ` [tarantool-patches] " Vladislav Shpilevoy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=af19ae87-e7af-c57d-8f3b-17c730325cad@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=kyukhin@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH 2/2] sql: replace KeyInfo with key_def' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox