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

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

Please, add to commit message results of benchmark to indicate
that this optimisation really matters.

> On 17 Jul 2018, at 00:27, Nikita Tatunov <hollow653@gmail.com> wrote:
> 
> diff --git a/src/box/sql.c b/src/box/sql.c
> index fdce224..398b2a6 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -1636,10 +1636,12 @@ sql_debug_info(struct info_handler *h)
>  	extern int sql_search_count;
>  	extern int sql_sort_count;
>  	extern int sql_found_count;
> +	extern int sql_xferOpt_count;

Don’t use camel notation. Lets call it simply ’sql_xfer_count’.

>  	info_begin(h);
>  	info_append_int(h, "sql_search_count", sql_search_count);
>  	info_append_int(h, "sql_sort_count", sql_sort_count);
>  	info_append_int(h, "sql_found_count", sql_found_count);
> +	info_append_int(h, "sql_xferOpt_count", sql_xferOpt_count);
>  	info_end(h);
>  }
>  
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 2c9188e..9a99bab 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -1635,7 +1635,7 @@ sqlite3OpenTableAndIndices(Parse * pParse,	/* Parsing context */
>   * purposes only - to make sure the transfer optimization really
>   * is happening when it is supposed to.
>   */
> -int sqlite3_xferopt_count;
> +int sql_xferOpt_count = 0;
>  #endif				/* SQLITE_TEST */
>  
>  #ifndef SQLITE_OMIT_XFER_OPT
> @@ -1658,6 +1658,8 @@ xferCompatibleIndex(Index * pDest, Index * pSrc)
>  	assert(pDest->pTable != pSrc->pTable);
>  	uint32_t nDestCol = index_column_count(pDest);
>  	uint32_t nSrcCol = index_column_count(pSrc);
> +	if ((pDest->idxType != pSrc->idxType))
> +		return 0;
>  	if (nDestCol != nSrcCol) {
>  		return 0;	/* Different number of columns */
>  	}
> @@ -1725,9 +1727,9 @@ 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();
> +	bool is_err_action_default = false;

Again: why do you need this flag? Default action is just synonym for ABORT,
so why should we care about it?

> +	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));

You don’t need to again proceed lookup: space is found few lines above.
Moreover, I see those lookups are executed inside ‘for' loop. Lets move
them outside it.

> +	struct index *src_idx = space_index(src_space, 0);
> +	struct index *dest_idx = space_index(dest_space, 0);
> +
> +	/* Xfer optimization is unable to correctly insert data
> +	 * in case there's a conflict action other than *_ABORT.
> +	 * This is the reason we want to only run it if the
> +	 * destination table is initially empty.
> +	 * That block generates code to make that determination.
> +	 */

Multi-line comment should be formatted as following:

/*
 * Comment starts here.
 * …
 */

> +
> +	if (!(onError == ON_CONFLICT_ACTION_ABORT &&
> +	    is_err_action_default == false)) {
>  		addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);
>  		VdbeCoverage(v);
>  		emptyDestTest = sqlite3VdbeAddOp0(v, OP_Goto);
>  		sqlite3VdbeJumpHere(v, addr1);
> +#ifdef SQLITE_TEST
> +		if (dest_idx->vtab->count(dest_idx, ITER_ALL, NULL, 0) == 0)
> +			sql_xferOpt_count++;

Actually, I don’t like this approach.
Look, query may be compiled and saved into cache (even thought it is still
not implemented yet). So it might be executed later and it might be not empty.
Moreover, we are going to avoid doing space lookups and use only def.
With only def you can’t execute count.

Personally, I wanted you to defer incrementing sql_xfer_count till
VDBE execution. For instance, you may add special flag and pass it
to OP_RowData indicating that xFer is currently processing.

> +#endif
> +	vdbe_emit_open_cursor(pParse, iSrc, 0, src_space);
> +	VdbeComment((v, "%s", src_idx->def->name));
> +	vdbe_emit_open_cursor(pParse, iDest, 0, dest_space);

I see few lines above:

sqlite3OpenTable(pParse, iDest, pDest, OP_OpenWrite);

So, basically you don’t need to open it again.

> +	VdbeComment((v, "%s", dest_idx->def->name));
> +	addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0);
> +	VdbeCoverage(v);
> +	sqlite3VdbeAddOp2(v, OP_RowData, iSrc, regData);
> +	sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData);
> +	sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
> +	sqlite3VdbeAddOp2(v, OP_Next, iSrc, addr1 + 1);
> +	VdbeCoverage(v);
> +	sqlite3VdbeJumpHere(v, addr1);
> +	sqlite3VdbeAddOp2(v, OP_Close, iSrc, 0);
> +	sqlite3VdbeAddOp2(v, OP_Close, iDest, 0);
> +
>  	if (emptySrcTest)
>  		sqlite3VdbeJumpHere(v, emptySrcTest);
>  	sqlite3ReleaseTempReg(pParse, regTupleid);
> 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..e75fabc
> --- /dev/null
> +test:do_catchsql_test(
> +	"xfer-optimization-1.15",
> +	[[
> +		DROP TABLE t1;
> +		DROP TABLE t2;
> +		CREATE TABLE t1(a INTEGER PRIMARY KEY, b UNIQUE);
> +		CREATE TABLE t2(a INTEGER PRIMARY KEY, b UNIQUE);
> +		INSERT INTO t1 VALUES (2, 2), (3, 3), (5, 5);
> +		INSERT INTO t2 VALUES (1, 1), (4, 4);
> +		INSERT OR ROLLBACK INTO t2 SELECT * FROM t1;

INSERT OT ROLLBACK outside transaction works the same as ABORT and DEFAULT.
So, surround it with transaction and check that it really rollbacks.

> +	]], {
> +		-- <xfer-optimization-1.15>
> +		0
> +		-- <xfer-optimization-1.15>
> +	})
> +
> +test:do_execsql_test(
> +	"xfer-optimization-1.16",
> +	[[
> +		SELECT * FROM t2;
> +	]], {
> +		-- <xfer-optimization-1.16>
> +		1, 1, 2, 2, 3, 3, 4, 4, 5, 5
> +		-- <xfer-optimization-1.16>
> +	})
> +
> +-- The following tests are supposed to test if xfer-optimization is actually
> +-- used in the given cases (if the conflict actually occurs):
> +-- 	1.0) insert w/o explicit confl. action & w/o index replace action
> +-- 	1.1) insert w/o explicit confl. action & w/ index replace action & empty dest_table
> +-- 	1.2) insert w/o explicit confl. action & w/ index replace action & non-empty dest_table
> +-- 	2) insert with abort
> +-- 	3.0) insert with rollback (into empty table)
> +-- 	3.1) insert with rollback (into non-empty table)
> +-- 	4) insert with replace
> +-- 	5) insert with fail
> +-- 	6) insert with ignore
> +
> +
> +-- 1.0) insert w/o explicit confl. action & w/o index replace action
> +-------------------------------------------------------------------------------------------
> +
> +bfr = box.sql.debug().sql_xferOpt_count
> +
> +test:do_catchsql_test(
> +	"xfer-optimization-1.17",
> +	[[
> +		DROP TABLE t1;
> +		DROP TABLE t2;
> +		CREATE TABLE t1(a INTEGER PRIMARY KEY, b);
> +		CREATE TABLE t2(a INTEGER PRIMARY KEY, b);
> +		INSERT INTO t1 VALUES (1, 1), (3, 3), (5, 5);
> +		INSERT INTO t2 VALUES (2, 2), (3, 4);
> +		BEGIN;
> +			INSERT INTO t2 VALUES (4, 4);
> +			INSERT INTO t2 SELECT * FROM t1;
> +	]], {
> +		-- <xfer-optimization-1.17>
> +		1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space 'T2'"
> +		-- <xfer-optimization-1.17>
> +	})
> +
> +test:do_execsql_test(
> +	"xfer-optimization-1.18",
> +	[[
> +			INSERT INTO t2 VALUES (10, 10);
> +		COMMIT;
> +		SELECT * FROM t2;
> +	]], {
> +		-- <xfer-optimization-1.18>
> +		2, 2, 3, 4, 4, 4, 10, 10
> +		-- <xfer-optimization-1.18>
> +	})
> +
> +aftr = box.sql.debug().sql_xferOpt_count
> +
> +test:do_test(
> +	"xfer-optimization-1.19",
> +	function()
> +		if (aftr - bfr == 1) then
> +			return {1}
> +		end
> +		if (aftr == bfr) then
> +			return {0}
> +		end
> +		return {2}

Why do you repeat this snippet each time? You can declare it as named
function once and use it everywhere.

> +	end, {
> +		-- <xfer-optimization-1.19>
> +		0
> +		-- <xfer-optimization-1.19>
> +	})
> +
> +-- 1.1) insert w/o explicit confl. action & w/ index replace action & empty dest_table

Even in tests lets not exceed 80 chars (here and in other places).