[tarantool-patches] Re: [PATCH] sql: xfer optimization issue
n.pettik
korablev at tarantool.org
Fri May 4 01:57:11 MSK 2018
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index ae8dafb..734ff34 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -1645,6 +1645,12 @@ xferCompatibleIndex(Index * pDest, Index * pSrc)
> assert(pDest->pTable != pSrc->pTable);
> uint32_t nDestCol = index_column_count(pDest);
> uint32_t nSrcCol = index_column_count(pSrc);
> + /* One of them is PK while the other isn't. */
> + if ((pDest->idxType == SQLITE_IDXTYPE_PRIMARYKEY &&
> + pSrc->idxType != SQLITE_IDXTYPE_PRIMARYKEY) ||
> + (pDest->idxType != SQLITE_IDXTYPE_PRIMARYKEY &&
> + pSrc->idxType == SQLITE_IDXTYPE_PRIMARYKEY))
> + return 0;
Why don’t just compare their types? Wouldn’t it be easier?
Yep, such condition would be little bit stronger, but I think it is worth it.
> @@ -1718,9 +1724,10 @@ xferOptimization(Parse * pParse, /* Parser context */
> int emptyDestTest = 0; /* Address of test for empty pDest */
> int emptySrcTest = 0; /* Address of test for empty pSrc */
> Vdbe *v; /* The VDBE we are building */
> - int destHasUniqueIdx = 0; /* True if pDest has a UNIQUE index */
> int regData, regTupleid; /* Registers holding data and tupleid */
> struct session *user_session = current_session();
> + int dest_has_replace_action = 0;
> + int confl_action_default = 0;
Call it error action. Also, it is better to use bool type
(even if bool might be really int underhood). And bool values usually
comes with ‘is_’ or ‘has_' prefix.
> @@ -1737,8 +1744,10 @@ xferOptimization(Parse * pParse, /* Parser context */
> if (onError == ON_CONFLICT_ACTION_DEFAULT) {
> if (pDest->iPKey >= 0)
> onError = pDest->keyConf;
> - if (onError == ON_CONFLICT_ACTION_DEFAULT)
> + if (onError == ON_CONFLICT_ACTION_DEFAULT) {
> onError = ON_CONFLICT_ACTION_ABORT;
> + confl_action_default = 1;
Why do you need this variable at all? I mean, DEFAULT always
is an alias to ABORT, isn’t it?
> + }
> }
> assert(pSelect->pSrc); /* allocated even if there is no FROM clause */
> if (pSelect->pSrc->nSrc != 1) {
> @@ -1828,15 +1837,16 @@ xferOptimization(Parse * pParse, /* Parser context */
> return 0; /* Default values must be the same for all columns */
> }
> }
> + if (pDestCol->notNull == ON_CONFLICT_ACTION_REPLACE)
> + dest_has_replace_action = 1;
Don’t confuse NOT NULL error action for field and conflict action for index:
Index->onError is not the same as Column->notNull.
> for (pDestIdx = pDest->pIndex; pDestIdx; pDestIdx = pDestIdx->pNext) {
> - if (index_is_unique(pDestIdx)) {
> - destHasUniqueIdx = 1;
> - }
> for (pSrcIdx = pSrc->pIndex; pSrcIdx; pSrcIdx = pSrcIdx->pNext) {
> if (xferCompatibleIndex(pDestIdx, pSrcIdx))
> break;
> }
> + if (pDestIdx->onError != ON_CONFLICT_ACTION_REPLACE)
> + dest_has_replace_action = 1;
Wait, you assign ’true’ to variable ‘has_replace’ when it is not really replace…
It looks very strange.
> @@ -1875,58 +1885,63 @@ xferOptimization(Parse * pParse, /* Parser context */
> +
> + /* Xfer optimization is unable to correctly insert
> + * data in case there's a conflict action
> + * other than ON_CONFLICT_ACTION_ROLLBACK or there's
> + * ON_CONFLICT_ACTION_DEFAULT which was transformed into
> + * ON_CONFLICT_ACTION_ABORT for insertion while we have a
> + * ON_CONFLICT_ACTION_REPLACE for any of constraints.
I would use only names of actions, these enum names are too long.
But it is up to you.
> + if (onError != ON_CONFLICT_ACTION_ROLLBACK ||
> + (confl_action_default == 1 &&
> + dest_has_replace_action == 1)) {
You can fit two lines above into one...
> + int space_ptr_reg = ++pParse->nMem;
> + struct space *src_space =
> + space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrc->tnum));
> + struct space *dest_space =
> + space_by_id(SQLITE_PAGENO_TO_SPACEID(pDest->tnum));
> + struct index *src_idx = space_index(src_space, 0);
> + struct index *dest_idx;
Why don’t make index lookup for destination space right here?
> + pDestIdx = sqlite3PrimaryKeyIndex(pDest);
> + pSrcIdx = sqlite3PrimaryKeyIndex(pSrc);
These variables are unused.
> diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
> new file mode 100755
> index 0000000..860b4c3
> --- /dev/null
> +++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
> @@ -0,0 +1,233 @@
> +#!/usr/bin/env tarantool
> +test = require("sqltester")
> +test:plan(18)
The only one thing I don’t really like in these tests is the absence
of verification of occurred optimisation. Could you come up with the way
how to check it?
More information about the Tarantool-patches
mailing list