[tarantool-patches] Re: [PATCH] sql: xfer optimization issue

["n.pettik" <korablev@tarantool.org>]

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