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

[n.pettik]

> Currently insertion from the table to another one
> with the same schema using SELECT works wrong.
> The problem lies in xfer optimization which opens cursors for
> all of the indexes and inserts data excessively.
> Now only PRIMARY KEY is used to handle insertion.
> Moreover analyzing xfer optimization I have noticed

Nitpicking: it is not worth mentioning process of exploration,
just use (in this particular case) statement of facts.

> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index ae8dafb..ed134f4 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -1711,6 +1711,7 @@ xferOptimization(Parse * pParse,	/* Parser context */
>  	ExprList *pEList;	/* The result set of the SELECT */
>  	Table *pSrc;		/* The table in the FROM clause of SELECT */
>  	Index *pSrcIdx, *pDestIdx;	/* Source and destination indices */
> +	struct index *src_idx, *dest_idx;	/* Source and destination indices */

We don’t use forward var declaration: it is obsolete rule from ancient C standards.
Moreover, we place comments above the code to be commented.

> +	struct space *space;	/* Space pointer for pDest and pSrc */

The same is here.

> -	assert(destHasUniqueIdx);
> -	if ((pDest->iPKey < 0 && pDest->pIndex != 0)	/* (1) */
> -	    ||destHasUniqueIdx	/* (2) */
> -	    || (onError != ON_CONFLICT_ACTION_ABORT
> -		&& onError != ON_CONFLICT_ACTION_ROLLBACK)	/* (3) */
> -	    ) {
> -		/* In some circumstances, we are able to run the xfer optimization
> -		 * only if the destination table is initially empty.
> -		 * This block generates code to make
> -		 * that determination.
> -		 *
> -		 * Conditions under which the destination must be empty:
> -		 *
> -		 * (1) There is no INTEGER PRIMARY KEY but there are indices.
> -		 *
> -		 * (2) The destination has a unique index.  (The xfer optimization
> -		 *     is unable to test uniqueness.)
> -		 *
> -		 * (3) onError is something other than ON_CONFLICT_ACTION_ABORT and _ROLLBACK.
> -		 */
> -		addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);
> -		VdbeCoverage(v);
> -		emptyDestTest = sqlite3VdbeAddOp0(v, OP_Goto);
> -		sqlite3VdbeJumpHere(v, addr1);
> -	}

Why did you delete if-clause? The code which checks that
table is empty is needed only in the cases in if-clauses,
i.e. in some cases xfer optimisation can be applied even if
table is not initially empty (as far as I understand).

> +	/* The xfer optimization is unable to test uniqueness
> +	 * while we have a unique PRIMARY KEY in any existing table.
> +	 * This is the reason we can only run it if the destination table

Nitpicking: comments should fit into 66 chars.

> +	 * is initially empty.
> +	 * This block generates code to make that determination.
> +	 */
> +	addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);
> +	VdbeCoverage(v);
> +	emptyDestTest = sqlite3VdbeAddOp0(v, OP_Goto);
> +	sqlite3VdbeJumpHere(v, addr1);
> +
> +	space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pDest->tnum));
> +	dest_idx = space_index(space, 0 /* PK */);
> +	space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrc->tnum));
> +	src_idx = space_index(space, 0 /* PK */);
> +	assert(src_idx);
> +	assert(dest_idx);

We use explicit != NULL checks.

> +	pDestIdx = sqlite3PrimaryKeyIndex(pDest);
> +	pSrcIdx = sqlite3PrimaryKeyIndex(pSrc);

In fact, you don’t need these indexes at all.
Instead of calling emit_open_cursor() you
can simply add two opcodes: OP_LoadPtr and OP_OpenWrite
(as it happens inside that function). Or, you can use pSrc->tnum.
Table’s tnum exactly encodes PK space id (index id is 0).
As for KeyInfo - you are able to remove it at all, since opcodes
below don’t rely on key info (AFAIK, better check it yourself).
The only issue remained - how to check PK compatibility.
I would add to xferCompatibleIndex() another one check:
If first index is PK, then second also must be PK.
It is easy to implement, since Index struct contains appropriate flag.