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

[n.pettik]

Hello. Consider comments below.

> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index ae8dafb..87e1bad 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -1645,6 +1645,9 @@ 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 == 2 && pSrc->idxType != 2)
> +	    || (pDest->idxType != 2 && pSrc->idxType == 2))

It is better to use SQLITE_IDXTYPE_PRIMARYKEY macros,
then hardcoded constants.

But the main thing: you forgot to add ‘return 0;’ statement here.

Codestyle nitpicking: put binary operator to previous line.
Also, put dots at the end of sentences.

>  	if (nDestCol != nSrcCol) {
>  		return 0;	/* Different number of columns */
>  	}
> @@ -1711,16 +1714,21 @@ 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 */
> +	/* Source and destination indices */
> +	struct index *src_idx, *dest_idx;

You didn’t fixed remark from previous review:
>We don’t use forward var declaration: it is obsolete rule from ancient C standards.
Although it is not mandatory, but better fix it.

>  	struct SrcList_item *pItem;	/* An element of pSelect->pSrc */
>  	int i;			/* Loop counter */
>  	int iSrc, iDest;	/* Cursors from source and destination */
>  	int addr1;		/* Loop addresses */
>  	int emptyDestTest = 0;	/* Address of test for empty pDest */
>  	int emptySrcTest = 0;	/* Address of test for empty pSrc */
> +	/* Number of memory cell for cursors */
> +	int space_ptr_reg;

The same is here.

>  	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();
> +	/* Space pointer for pDest and pSrc */
> +	struct space *space;

And here.

> +	/* The xfer optimization is unable to test
> +	 * uniqueness while we have a unique
> +	 * PRIMARY KEY in any existing table.

Rephrase this sentence. Or I have missed smth:
uniqness is checked while tuple is inserted in space by
Tarantool facilities, isn’t it? For this reason, you can try to return
those if-checks for additional code-generation.
The only concern I have is about ‘on conflict replace'
error action: if source table isn’t initially empty, features 'on replace'
error action and you insert table which violates uniqueness - 
this replace action might fail.
I advise you to investigate such behaviour.

> +	 * This is the reason we can only run it
> +	 * if the destination table 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);
> +
> +	pDestIdx = sqlite3PrimaryKeyIndex(pDest);
> +	pSrcIdx = sqlite3PrimaryKeyIndex(pSrc);
> +	space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrc->tnum));
> +	if((src_idx = space_index(space, 0 /* PK */)) == NULL)

Codestyle nitpicking: put space after ‘if’.
Also, do not ‘inline’ comments into code.
And why space can’t feature primary index?
AFAIK, only views don’t have PK, but src table is tested
to be real table:
	if (space_is_view(pSrc)) {
		return 0;	/* tab2 may not be a view */
	} 

> +		return 0;
> +	space_ptr_reg = ++pParse->nMem;
> +	sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0,
> +			     (void *) space);
> +	sqlite3VdbeAddOp3(v, OP_OpenWrite, iSrc,
> +			  pSrc->tnum, space_ptr_reg);
> +	VdbeComment((v, "%s", src_idx->def->name));
> +	space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pDest->tnum));

Don’t be afraid of introducing new variables.
It would be better, if you use src_space and dest_space separately.