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

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

[N.Tatunov]

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.
The bug was fixed so the data should now insert
correctly.

Closes #3307
---

Branch: https://github.com/tarantool/tarantool/tree/N_Tatunov/gh-3307-xfer-optimization-issue
Issue: https://github.com/tarantool/tarantool/issues/3307

 src/box/sql/insert.c                               | 35 ++++++++--------
 .../sql-tap/gh-3307-xfer-optimization-issue.result |  0
 .../gh-3307-xfer-optimization-issue.test.lua       | 49 ++++++++++++++++++++++
 3 files changed, 67 insertions(+), 17 deletions(-)
 create mode 100644 test/sql-tap/gh-3307-xfer-optimization-issue.result
 create mode 100644 test/sql-tap/gh-3307-xfer-optimization-issue.test.lua

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index ae8dafb..b27fc23 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1908,24 +1908,25 @@ xferOptimization(Parse * pParse,	/* Parser context */
 				break;
 		}
 		assert(pSrcIdx);
-		emit_open_cursor(pParse, iSrc, pSrcIdx->tnum);
-		sqlite3VdbeSetP4KeyInfo(pParse, pSrcIdx);
-		VdbeComment((v, "%s", pSrcIdx->zName));
-		emit_open_cursor(pParse, iDest, pDestIdx->tnum);
-		sqlite3VdbeSetP4KeyInfo(pParse, pDestIdx);
-		sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR);
-		VdbeComment((v, "%s", pDestIdx->zName));
-		addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0);
-		VdbeCoverage(v);
-		sqlite3VdbeAddOp2(v, OP_RowData, iSrc, regData);
-		sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData);
-		if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY)
+		if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY) {
+			emit_open_cursor(pParse, iSrc, pSrcIdx->tnum);
+			sqlite3VdbeSetP4KeyInfo(pParse, pSrcIdx);
+			VdbeComment((v, "%s", pSrcIdx->zName));
+			emit_open_cursor(pParse, iDest, pDestIdx->tnum);
+			sqlite3VdbeSetP4KeyInfo(pParse, pDestIdx);
+			sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR);
+			VdbeComment((v, "%s", pDestIdx->zName));
+			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);
+			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);
diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.result b/test/sql-tap/gh-3307-xfer-optimization-issue.result
new file mode 100644
index 0000000..e69de29
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 100644
index 0000000..e45235e
--- /dev/null
+++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
@@ -0,0 +1,49 @@
+#!/usr/bin/env tarantool
+test = require("sqltester")
+test:plan(1)
+
+-- gh-3307 - sql: INSERT with SELECT is not working in some cases.
+
+test:do_exesql_test(
+	"xfer-optimization-1.1",
+	[[
+		CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
+		INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+		CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
+		INSERT INTO t2 SELECT * FROM t1;
+		DROP TABLE t1;
+		DROP TABLE t2;
+	]], {
+		-- <xfer-optimization-1.1>
+		-- <xfer-optimization-1.1>
+	})
+
+test:do_exesql_test(
+	"xfer-optimization-1.2",
+	[[
+		CREATE TABLE t1(id INTEGER PRIMARY KEY, b INTEGER);
+		CREATE TABLE t2(id INTEGER PRIMARY KEY, b INTEGER);
+		CREATE INDEX i1 ON t1(b);
+		CREATE INDEX i2 ON t2(b);
+		INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+		INSERT INTO t2 SELECT * FROM t1;
+		DROP TABLE t1;
+		DROP TABLE t2;
+	]], {
+		-- <xfer-optimization-1.2>
+		-- <xfer-optimization-1.2>
+	})
+		
+test:do_exesql_test(
+	"xfer-optimization-1.1",
+	[[
+		CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
+		INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+		CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER);
+		INSERT INTO t2 SELECT * FROM t1;
+		DROP TABLE t1;
+		DROP TABLE t2;
+	]], {
+		-- <xfer-optimization-1.1>
+		-- <xfer-optimization-1.1>
+	})
-- 
2.7.4

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

[Hollow111]

[-- Attachment #1: Type: text/plain, Size: 9487 bytes --]

Hello. I have notices some mistakes.
Newer diff:

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index ae8dafb..b27fc23 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1908,24 +1908,25 @@ xferOptimization(Parse * pParse, /* Parser context
*/
  break;
  }
  assert(pSrcIdx);
- emit_open_cursor(pParse, iSrc, pSrcIdx->tnum);
- sqlite3VdbeSetP4KeyInfo(pParse, pSrcIdx);
- VdbeComment((v, "%s", pSrcIdx->zName));
- emit_open_cursor(pParse, iDest, pDestIdx->tnum);
- sqlite3VdbeSetP4KeyInfo(pParse, pDestIdx);
- sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR);
- VdbeComment((v, "%s", pDestIdx->zName));
- addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0);
- VdbeCoverage(v);
- sqlite3VdbeAddOp2(v, OP_RowData, iSrc, regData);
- sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData);
- if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY)
+ if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY) {
+ emit_open_cursor(pParse, iSrc, pSrcIdx->tnum);
+ sqlite3VdbeSetP4KeyInfo(pParse, pSrcIdx);
+ VdbeComment((v, "%s", pSrcIdx->zName));
+ emit_open_cursor(pParse, iDest, pDestIdx->tnum);
+ sqlite3VdbeSetP4KeyInfo(pParse, pDestIdx);
+ sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR);
+ VdbeComment((v, "%s", pDestIdx->zName));
+ 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);
+ 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);
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..3b2bcc6
--- /dev/null
+++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
@@ -0,0 +1,52 @@
+#!/usr/bin/env tarantool
+test = require("sqltester")
+test:plan(3)
+
+test:do_execsql_test(
+ "xfer-optimization-1.1",
+ [[
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
+ INSERT INTO t2 SELECT * FROM t1;
+ DROP TABLE t1;
+ DROP TABLE t2;
+ ]], {
+ -- <xfer-optimization-1.1>
+
+ -- <xfer-optimization-1.1>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.2",
+ [[
+ CREATE TABLE t1(id INTEGER PRIMARY KEY, b INTEGER);
+ CREATE TABLE t2(id INTEGER PRIMARY KEY, b INTEGER);
+ CREATE INDEX i1 ON t1(b);
+ CREATE INDEX i2 ON t2(b);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ INSERT INTO t2 SELECT * FROM t1;
+ DROP TABLE t1;
+ DROP TABLE t2;
+ ]], {
+ -- <xfer-optimization-1.2>
+
+ -- <xfer-optimization-1.2>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.3",
+ [[
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t2 SELECT * FROM t1;
+ DROP TABLE t1;
+ DROP TABLE t2;
+ ]], {
+ -- <xfer-optimization-1.3>
+
+ -- <xfer-optimization-1.3>
+ })
+
+test:finish_test()


ср, 18 апр. 2018 г. в 18:32, N.Tatunov <hollow653@gmail.com>:

> 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.
> The bug was fixed so the data should now insert
> correctly.
>
> Closes #3307
> ---
>
> Branch:
> https://github.com/tarantool/tarantool/tree/N_Tatunov/gh-3307-xfer-optimization-issue
> Issue: https://github.com/tarantool/tarantool/issues/3307
>
>  src/box/sql/insert.c                               | 35 ++++++++--------
>  .../sql-tap/gh-3307-xfer-optimization-issue.result |  0
>  .../gh-3307-xfer-optimization-issue.test.lua       | 49
> ++++++++++++++++++++++
>  3 files changed, 67 insertions(+), 17 deletions(-)
>  create mode 100644 test/sql-tap/gh-3307-xfer-optimization-issue.result
>  create mode 100644 test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
>
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index ae8dafb..b27fc23 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -1908,24 +1908,25 @@ xferOptimization(Parse * pParse,        /* Parser
> context */
>                                 break;
>                 }
>                 assert(pSrcIdx);
> -               emit_open_cursor(pParse, iSrc, pSrcIdx->tnum);
> -               sqlite3VdbeSetP4KeyInfo(pParse, pSrcIdx);
> -               VdbeComment((v, "%s", pSrcIdx->zName));
> -               emit_open_cursor(pParse, iDest, pDestIdx->tnum);
> -               sqlite3VdbeSetP4KeyInfo(pParse, pDestIdx);
> -               sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR);
> -               VdbeComment((v, "%s", pDestIdx->zName));
> -               addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0);
> -               VdbeCoverage(v);
> -               sqlite3VdbeAddOp2(v, OP_RowData, iSrc, regData);
> -               sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData);
> -               if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY)
> +               if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY) {
> +                       emit_open_cursor(pParse, iSrc, pSrcIdx->tnum);
> +                       sqlite3VdbeSetP4KeyInfo(pParse, pSrcIdx);
> +                       VdbeComment((v, "%s", pSrcIdx->zName));
> +                       emit_open_cursor(pParse, iDest, pDestIdx->tnum);
> +                       sqlite3VdbeSetP4KeyInfo(pParse, pDestIdx);
> +                       sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR);
> +                       VdbeComment((v, "%s", pDestIdx->zName));
> +                       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);
> +                       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);
> diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.result
> b/test/sql-tap/gh-3307-xfer-optimization-issue.result
> new file mode 100644
> index 0000000..e69de29
> 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 100644
> index 0000000..e45235e
> --- /dev/null
> +++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
> @@ -0,0 +1,49 @@
> +#!/usr/bin/env tarantool
> +test = require("sqltester")
> +test:plan(1)
> +
> +-- gh-3307 - sql: INSERT with SELECT is not working in some cases.
> +
> +test:do_exesql_test(
> +       "xfer-optimization-1.1",
> +       [[
> +               CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
> +               INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
> +               CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
> +               INSERT INTO t2 SELECT * FROM t1;
> +               DROP TABLE t1;
> +               DROP TABLE t2;
> +       ]], {
> +               -- <xfer-optimization-1.1>
> +               -- <xfer-optimization-1.1>
> +       })
> +
> +test:do_exesql_test(
> +       "xfer-optimization-1.2",
> +       [[
> +               CREATE TABLE t1(id INTEGER PRIMARY KEY, b INTEGER);
> +               CREATE TABLE t2(id INTEGER PRIMARY KEY, b INTEGER);
> +               CREATE INDEX i1 ON t1(b);
> +               CREATE INDEX i2 ON t2(b);
> +               INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
> +               INSERT INTO t2 SELECT * FROM t1;
> +               DROP TABLE t1;
> +               DROP TABLE t2;
> +       ]], {
> +               -- <xfer-optimization-1.2>
> +               -- <xfer-optimization-1.2>
> +       })
> +
> +test:do_exesql_test(
> +       "xfer-optimization-1.1",
> +       [[
> +               CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
> +               INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
> +               CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER);
> +               INSERT INTO t2 SELECT * FROM t1;
> +               DROP TABLE t1;
> +               DROP TABLE t2;
> +       ]], {
> +               -- <xfer-optimization-1.1>
> +               -- <xfer-optimization-1.1>
> +       })
> --
> 2.7.4
>
>

[-- Attachment #2: Type: text/html, Size: 15258 bytes --]

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

[n.pettik]

>The bug was fixed so the data should now insert
>correctly.

Please, instead of mentioning that you just fixed bug (it is obvious),
provide brief information (without digging in details) how the problem was solved. 
(e.g. 'now only PK is used to handle insertion').

Overall, the idea is OK, but implementation could be more elegant.
You don’t need to iterate through all dest/source indexes:
it is possible to get PK using function sqlite3PrimaryKeyIndex();
Thus, complexity reduces from O(n^2) to O(n), where n - number of indexes.

But, there is even better approach: in Tarantool PK always comes with 0 ordinal
number. So, you can do space lookup by id (there is macros, which converts
table->tnum to space id: SQLITE_PAGENO_TO_SPACEID) and fetch real PK
index with O(1) complexity: space_index(space, 0 /* PK */);
It is not mandatory now, only if you are willing to do it.

Also, as we have discussed, remove pls redundant uniqueness check.

>  	}
>  	if (emptySrcTest)
>  		sqlite3VdbeJumpHere(v, emptySrcTest);
> 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..3b2bcc6
> --- /dev/null
> +++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
> @@ -0,0 +1,52 @@
> +#!/usr/bin/env tarantool
> +test = require("sqltester")
> +test:plan(3)
> +
> +test:do_execsql_test(
> +	"xfer-optimization-1.1",
> +	[[
> +		CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
> +		INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
> +		CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
> +		INSERT INTO t2 SELECT * FROM t1;
> +		DROP TABLE t1;
> +		DROP TABLE t2;
> +	]], {
> +		-- <xfer-optimization-1.1>
> +
> +		-- <xfer-optimization-1.1>
> +	})

do_execsql_test() returns result of last executed query.
In this case, it is ‘DROP TABLE’, which always (in this particular case)
will return nothing (i.e. table will be successfully dropped).
To catch some error, you can use do_catchsql_test() function.
After you check that insertion occurs without errors, you need
to check that all rows have been transferred from one table to another.
So, you just use do_execsql_test() to test 'SELECT * FROM t2;’.
After all, you may drop tables in the beginning of next test,
since it won’t affect result of last executed statement.

Moreover, I would add more test cases to verify that xfer
optimization in general works: try to rearrange columns/indexes
order, add different ON CONFLICT clauses etc.

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

[Hollow111]

[-- Attachment #1: Type: text/plain, Size: 12305 bytes --]

Fixes were made:

чт, 19 апр. 2018 г. в 14:22, n.pettik <korablev@tarantool.org>:

> >The bug was fixed so the data should now insert
> >correctly.
>
> Please, instead of mentioning that you just fixed bug (it is obvious),
> provide brief information (without digging in details) how the problem was
> solved.
> (e.g. 'now only PK is used to handle insertion').
>
> Overall, the idea is OK, but implementation could be more elegant.
> You don’t need to iterate through all dest/source indexes:
> it is possible to get PK using function sqlite3PrimaryKeyIndex();
> Thus, complexity reduces from O(n^2) to O(n), where n - number of indexes.
>
> But, there is even better approach: in Tarantool PK always comes with 0
> ordinal
> number. So, you can do space lookup by id (there is macros, which converts
> table->tnum to space id: SQLITE_PAGENO_TO_SPACEID) and fetch real PK
> index with O(1) complexity: space_index(space, 0 /* PK */);
> It is not mandatory now, only if you are willing to do it.
>
> Also, as we have discussed, remove pls redundant uniqueness check.
>
> >       }
> >       if (emptySrcTest)
> >               sqlite3VdbeJumpHere(v, emptySrcTest);
> > 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..3b2bcc6
> > --- /dev/null
> > +++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
> > @@ -0,0 +1,52 @@
> > +#!/usr/bin/env tarantool
> > +test = require("sqltester")
> > +test:plan(3)
> > +
> > +test:do_execsql_test(
> > +     "xfer-optimization-1.1",
> > +     [[
> > +             CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
> > +             INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
> > +             CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
> > +             INSERT INTO t2 SELECT * FROM t1;
> > +             DROP TABLE t1;
> > +             DROP TABLE t2;
> > +     ]], {
> > +             -- <xfer-optimization-1.1>
> > +
> > +             -- <xfer-optimization-1.1>
> > +     })
>
> do_execsql_test() returns result of last executed query.
> In this case, it is ‘DROP TABLE’, which always (in this particular case)
> will return nothing (i.e. table will be successfully dropped).
> To catch some error, you can use do_catchsql_test() function.
> After you check that insertion occurs without errors, you need
> to check that all rows have been transferred from one table to another.
> So, you just use do_execsql_test() to test 'SELECT * FROM t2;’.
> After all, you may drop tables in the beginning of next test,
> since it won’t affect result of last executed statement.
>
> Moreover, I would add more test cases to verify that xfer
> optimization in general works: try to rearrange columns/indexes
> order, add different ON CONFLICT clauses etc.
>
>
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
that unnecessary check from sqlite3 were used,
accordingly excessive code cencerned with it
was deleted.

Closes #3307
---

Branch:
https://github.com/tarantool/tarantool/tree/N_Tatunov/gh-3307-xfer-optimization-issue
Issue: https://github.com/tarantool/tarantool/issues/3307

 src/box/sql/insert.c                               |  64 ++++-----
 .../gh-3307-xfer-optimization-issue.test.lua       | 159
+++++++++++++++++++++
 2 files changed, 184 insertions(+), 39 deletions(-)
 create mode 100755 test/sql-tap/gh-3307-xfer-optimization-issue.test.lua

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 */
  struct SrcList_item *pItem; /* An element of pSelect->pSrc */
  int i; /* Loop counter */
  int iSrc, iDest; /* Cursors from source and destination */
@@ -1718,9 +1719,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();
+ struct space *space; /* Space pointer for pDest and pSrc */

  if (pSelect == NULL)
  return 0; /* Must be of the form  INSERT INTO ... SELECT ... */
@@ -1830,9 +1831,6 @@ xferOptimization(Parse * pParse, /* Parser context */
  }
  }
  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;
@@ -1875,52 +1873,40 @@ xferOptimization(Parse * pParse, /* Parser context
*/
  regData = sqlite3GetTempReg(pParse);
  regTupleid = sqlite3GetTempReg(pParse);
  sqlite3OpenTable(pParse, iDest, pDest, OP_OpenWrite);
- 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);
- }

- for (pDestIdx = pDest->pIndex; pDestIdx; pDestIdx = pDestIdx->pNext) {
- for (pSrcIdx = pSrc->pIndex; ALWAYS(pSrcIdx);
-      pSrcIdx = pSrcIdx->pNext) {
- if (xferCompatibleIndex(pDestIdx, pSrcIdx))
- break;
- }
+ /* 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
+ * 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);
+ pDestIdx = sqlite3PrimaryKeyIndex(pDest);
+ pSrcIdx = sqlite3PrimaryKeyIndex(pSrc);
+ if (xferCompatibleIndex(pDestIdx, pSrcIdx)) {
  assert(pSrcIdx);
  emit_open_cursor(pParse, iSrc, pSrcIdx->tnum);
  sqlite3VdbeSetP4KeyInfo(pParse, pSrcIdx);
- VdbeComment((v, "%s", pSrcIdx->zName));
+ VdbeComment((v, "%s", src_idx->def->name));
  emit_open_cursor(pParse, iDest, pDestIdx->tnum);
  sqlite3VdbeSetP4KeyInfo(pParse, pDestIdx);
  sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR);
- VdbeComment((v, "%s", pDestIdx->zName));
+ 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);
- if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY)
- sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+ sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
  sqlite3VdbeAddOp2(v, OP_Next, iSrc, addr1 + 1);
  VdbeCoverage(v);
  sqlite3VdbeJumpHere(v, addr1);
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..1049621
--- /dev/null
+++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
@@ -0,0 +1,159 @@
+#!/usr/bin/env tarantool
+test = require("sqltester")
+test:plan(12)
+
+test:do_catchsql_test(
+ "xfer-optimization-1.1",
+ [[
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.1>
+ 0
+ -- <xfer-optimization-1.1>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.2",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-oprimization-1.2>
+ 1, 1, 2, 2, 3, 3
+ -- <xfer-oprimization-1.2>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.3",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(id INTEGER PRIMARY KEY, b INTEGER);
+ CREATE TABLE t2(id INTEGER PRIMARY KEY, b INTEGER);
+ CREATE INDEX i1 ON t1(b);
+ CREATE INDEX i2 ON t2(b);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.3>
+ 0
+ -- <xfer-optimization-1.3>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.4",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.4>
+ 1, 1, 2, 2, 3, 3
+ -- <xfer-optimization-1.4>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.5",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER, c INTEGER);
+ INSERT INTO t1 VALUES (1, 1, 2), (2, 2, 3), (3, 3, 4);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t2 SELECT * FROM t1;
+
+ ]], {
+ -- <xfer-optimization-1.5>
+ 1, "table T2 has 2 columns but 3 values were supplied"
+ -- <xfer-optimization-1.5>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.6",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-oprimization-1.6>
+
+ -- <xfer-oprimization-1.6>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.7",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t2 SELECT * FROM t1;
+
+ ]], {
+ -- <xfer-optimization-1.7>
+ 0
+ -- <xfer-optimization-1.7>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.8",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-oprimization-1.6>
+ 1, 1, 2, 2, 3, 3
+ -- <xfer-oprimization-1.6>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.9",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 2);
+ CREATE TABLE t2(b INTEGER, a INTEGER PRIMARY KEY);
+ INSERT INTO t2 SELECT * FROM t1;
+
+ ]], {
+ -- <xfer-optimization-1.9>
+ 1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space
'T2'"
+ -- <xfer-optimization-1.9>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.10",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-oprimization-1.10>
+
+ -- <xfer-oprimization-1.10>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.11",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 2);
+ CREATE TABLE t2(b INTEGER PRIMARY KEY, a INTEGER);
+ INSERT INTO t2 SELECT * FROM t1;
+
+ ]], {
+ -- <xfer-optimization-1.11>
+ 0
+ -- <xfer-optimization-1.11>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.12",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-oprimization-1.12>
+ 1, 1, 2, 2, 3, 2
+ -- <xfer-oprimization-1.12>
+ })
+
+test:finish_test()
-- 
2.7.4

[-- Attachment #2: Type: text/html, Size: 25713 bytes --]

[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.

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

[Hollow111]

[-- Attachment #1: Type: text/plain, Size: 4989 bytes --]

пт, 20 апр. 2018 г. в 4:02, n.pettik <korablev@tarantool.org>:

>
> > 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).
>

Yes, that's true this code is needed if any of the
cases in if-clause != 0. But the point is that in Tarantool
there's always a PRIMARY KEY that is obviously unique
which means that the second case is always != 0 and
the result of the boolean expression is always not 0,
hence there's no reason for this if-check.


> > +     /* 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.
>
>

[-- Attachment #2: Type: text/html, Size: 6079 bytes --]

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

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 474 bytes --]


> On 20 Apr 2018, at 18:09, Hollow111 <hollow653@gmail.com> wrote:
> 
> Yes, that's true this code is needed if any of the 
> cases in if-clause != 0. But the point is that in Tarantool
> there's always a PRIMARY KEY that is obviously unique
> which means that the second case is always != 0 and
> the result of the boolean expression is always not 0,
> hence there's no reason for this if-check.

Ok, I got it. You are right: I misunderstood conditions under if-clause.



[-- Attachment #2: Type: text/html, Size: 1380 bytes --]

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

[Hollow111]

[-- Attachment #1: Type: text/plain, Size: 9648 bytes --]

Diff for the fixed patch:

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

  if (pSelect == NULL)
  return 0; /* Must be of the form  INSERT INTO ... SELECT ... */
@@ -1830,9 +1838,6 @@ xferOptimization(Parse * pParse, /* Parser context */
  }
  }
  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;
@@ -1875,58 +1880,51 @@ xferOptimization(Parse * pParse, /* Parser context
*/
  regData = sqlite3GetTempReg(pParse);
  regTupleid = sqlite3GetTempReg(pParse);
  sqlite3OpenTable(pParse, iDest, pDest, OP_OpenWrite);
- 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);
- }

- for (pDestIdx = pDest->pIndex; pDestIdx; pDestIdx = pDestIdx->pNext) {
- for (pSrcIdx = pSrc->pIndex; ALWAYS(pSrcIdx);
-      pSrcIdx = pSrcIdx->pNext) {
- if (xferCompatibleIndex(pDestIdx, pSrcIdx))
- break;
- }
- assert(pSrcIdx);
- emit_open_cursor(pParse, iSrc, pSrcIdx->tnum);
- sqlite3VdbeSetP4KeyInfo(pParse, pSrcIdx);
- VdbeComment((v, "%s", pSrcIdx->zName));
- emit_open_cursor(pParse, iDest, pDestIdx->tnum);
- sqlite3VdbeSetP4KeyInfo(pParse, pDestIdx);
- sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR);
- VdbeComment((v, "%s", pDestIdx->zName));
- addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0);
- VdbeCoverage(v);
- sqlite3VdbeAddOp2(v, OP_RowData, iSrc, regData);
- sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData);
- if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY)
- 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);
- }
+ /* 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 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)
+ 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));
+ if((dest_idx = space_index(space, 0 /* PK */)) == NULL)
+ return 0;
+ space_ptr_reg = ++pParse->nMem;
+ sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0,
+      (void *) space);
+ sqlite3VdbeAddOp3(v, OP_OpenWrite, iDest,
+   pSrc->tnum, space_ptr_reg);
+ sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR);
+ 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..0824c67
--- /dev/null
+++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
@@ -0,0 +1,155 @@
+#!/usr/bin/env tarantool
+test = require("sqltester")
+test:plan(12)
+
+test:do_catchsql_test(
+ "xfer-optimization-1.1",
+ [[
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.1>
+ 0
+ -- <xfer-optimization-1.1>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.2",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-oprimization-1.2>
+ 1, 1, 2, 2, 3, 3
+ -- <xfer-oprimization-1.2>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.3",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(id INTEGER PRIMARY KEY, b INTEGER);
+ CREATE TABLE t2(id INTEGER PRIMARY KEY, b INTEGER);
+ CREATE INDEX i1 ON t1(b);
+ CREATE INDEX i2 ON t2(b);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.3>
+ 0
+ -- <xfer-optimization-1.3>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.4",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.4>
+ 1, 1, 2, 2, 3, 3
+ -- <xfer-optimization-1.4>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.5",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER, c INTEGER);
+ INSERT INTO t1 VALUES (1, 1, 2), (2, 2, 3), (3, 3, 4);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.5>
+ 1, "table T2 has 2 columns but 3 values were supplied"
+ -- <xfer-optimization-1.5>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.6",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-oprimization-1.6>
+
+ -- <xfer-oprimization-1.6>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.7",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.7>
+ 0
+ -- <xfer-optimization-1.7>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.8",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-oprimization-1.6>
+ 1, 1, 2, 2, 3, 3
+ -- <xfer-oprimization-1.6>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.9",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 2);
+ CREATE TABLE t2(b INTEGER, a INTEGER PRIMARY KEY);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.9>
+ 1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space
'T2'"
+ -- <xfer-optimization-1.9>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.10",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-oprimization-1.10>
+
+ -- <xfer-oprimization-1.10>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.11",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 2);
+ CREATE TABLE t2(b INTEGER PRIMARY KEY, a INTEGER);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.11>
+ 0
+ -- <xfer-optimization-1.11>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.12",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-oprimization-1.12>
+ 1, 1, 2, 2, 3, 2
+ -- <xfer-oprimization-1.12>
+ })
+
+test:finish_test()


>

[-- Attachment #2: Type: text/html, Size: 24176 bytes --]

[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.

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

[Hollow111]

[-- Attachment #1: Type: text/plain, Size: 12237 bytes --]

Hello. I considered your remarks and the following changes were made:

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;
  if (nDestCol != nSrcCol) {
  return 0; /* Different number of columns */
  }
@@ -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;

  if (pSelect == NULL)
  return 0; /* Must be of the form  INSERT INTO ... SELECT ... */
@@ -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;
+ }
  }
  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;
  }
  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;
  if (pSrcIdx == 0) {
  return 0; /* pDestIdx has no corresponding index in pSrc */
  }
@@ -1875,58 +1885,63 @@ xferOptimization(Parse * pParse, /* Parser context
*/
  regData = sqlite3GetTempReg(pParse);
  regTupleid = sqlite3GetTempReg(pParse);
  sqlite3OpenTable(pParse, iDest, pDest, OP_OpenWrite);
- 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.
- */
+
+ /* 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.
+ * 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.
+ */
+
+ if (onError != ON_CONFLICT_ACTION_ROLLBACK ||
+     (confl_action_default == 1 &&
+     dest_has_replace_action == 1)) {
  addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);
  VdbeCoverage(v);
  emptyDestTest = sqlite3VdbeAddOp0(v, OP_Goto);
  sqlite3VdbeJumpHere(v, addr1);
  }

- for (pDestIdx = pDest->pIndex; pDestIdx; pDestIdx = pDestIdx->pNext) {
- for (pSrcIdx = pSrc->pIndex; ALWAYS(pSrcIdx);
-      pSrcIdx = pSrcIdx->pNext) {
- if (xferCompatibleIndex(pDestIdx, pSrcIdx))
- break;
- }
- assert(pSrcIdx);
- emit_open_cursor(pParse, iSrc, pSrcIdx->tnum);
- sqlite3VdbeSetP4KeyInfo(pParse, pSrcIdx);
- VdbeComment((v, "%s", pSrcIdx->zName));
- emit_open_cursor(pParse, iDest, pDestIdx->tnum);
- sqlite3VdbeSetP4KeyInfo(pParse, pDestIdx);
- sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR);
- VdbeComment((v, "%s", pDestIdx->zName));
- addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0);
- VdbeCoverage(v);
- sqlite3VdbeAddOp2(v, OP_RowData, iSrc, regData);
- sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData);
- if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY)
- 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);
- }
+ 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;
+
+ pDestIdx = sqlite3PrimaryKeyIndex(pDest);
+ pSrcIdx = sqlite3PrimaryKeyIndex(pSrc);
+ space_ptr_reg = ++pParse->nMem;
+ sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0,
+      (void *) src_space);
+ sqlite3VdbeAddOp3(v, OP_OpenWrite, iSrc,
+   pSrc->tnum, space_ptr_reg);
+ VdbeComment((v, "%s", src_idx->def->name));
+
+ if((dest_idx = space_index(dest_space, 0)) == NULL)
+ return 0;
+ space_ptr_reg = ++pParse->nMem;
+ sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0,
+      (void *) dest_space);
+ sqlite3VdbeAddOp3(v, OP_OpenWrite, iDest,
+   pSrc->tnum, space_ptr_reg);
+ sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR);
+ 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..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)
+
+test:do_catchsql_test(
+ "xfer-optimization-1.1",
+ [[
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.1>
+ 0
+ -- <xfer-optimization-1.1>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.2",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-oprimization-1.2>
+ 1, 1, 2, 2, 3, 3
+ -- <xfer-oprimization-1.2>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.3",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(id INTEGER PRIMARY KEY, b INTEGER);
+ CREATE TABLE t2(id INTEGER PRIMARY KEY, b INTEGER);
+ CREATE INDEX i1 ON t1(b);
+ CREATE INDEX i2 ON t2(b);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.3>
+ 0
+ -- <xfer-optimization-1.3>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.4",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.4>
+ 1, 1, 2, 2, 3, 3
+ -- <xfer-optimization-1.4>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.5",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER, c INTEGER);
+ INSERT INTO t1 VALUES (1, 1, 2), (2, 2, 3), (3, 3, 4);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.5>
+ 1, "table T2 has 2 columns but 3 values were supplied"
+ -- <xfer-optimization-1.5>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.6",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-oprimization-1.6>
+
+ -- <xfer-oprimization-1.6>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.7",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.7>
+ 0
+ -- <xfer-optimization-1.7>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.8",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-oprimization-1.6>
+ 1, 1, 2, 2, 3, 3
+ -- <xfer-oprimization-1.6>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.9",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 2);
+ CREATE TABLE t2(b INTEGER, a INTEGER PRIMARY KEY);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.9>
+ 1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space
'T2'"
+ -- <xfer-optimization-1.9>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.10",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-oprimization-1.10>
+
+ -- <xfer-oprimization-1.10>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.11",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 2);
+ CREATE TABLE t2(b INTEGER PRIMARY KEY, a INTEGER);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.11>
+ 0
+ -- <xfer-optimization-1.11>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.12",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-oprimization-1.12>
+ 1, 1, 2, 2, 3, 2
+ -- <xfer-oprimization-1.12>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.13",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b UNIQUE ON CONFLICT REPLACE);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b UNIQUE ON CONFLICT REPLACE);
+ INSERT INTO t1 VALUES (3, 3), (4, 4), (5, 5);
+ INSERT INTO t2 VALUES (1, 1), (2, 2);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.13>
+ 0
+ -- <xfer-optimization-1.13>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.14",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.14>
+ 1, 1, 2, 2, 3, 3, 4, 4, 5, 5
+ -- <xfer-optimization-1.14>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.15",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b UNIQUE ON CONFLICT REPLACE);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b UNIQUE ON CONFLICT REPLACE);
+ 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;
+ ]], {
+ -- <xfer-optimization-1.15>
+ 0
+ -- <xfer-optimization-1.15>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.16",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-oprimization-1.16>
+ 1, 1, 2, 2, 3, 3, 4, 4, 5, 5
+ -- <xfer-oprimization-1.16>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.17",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b UNIQUE ON CONFLICT REPLACE);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b UNIQUE ON CONFLICT REPLACE);
+ INSERT INTO t1 VALUES (1, 2), (3, 3), (5, 5);
+ INSERT INTO t2 VALUES (1, 1), (4, 4);
+ INSERT OR REPLACE INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.17>
+ 0
+ -- <xfer-optimization-1.17>
+ })
+
+test:do_execsql_test(
+ "xfer-oprimization-1.18",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-oprimization-1.18>
+ 1, 2, 3, 3, 4, 4, 5, 5
+ -- <xfer-oprimization-1.18>
+ })
+
+test:finish_test()

[-- Attachment #2: Type: text/html, Size: 30805 bytes --]

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

[n.pettik]

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

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

[Hollow111]

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

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

Yes, it is, but there's a little difference between directly specified
ABORT for an
insert stmt (INSERT OR ABORT) and just INSERT without any specified error
action
(ABORT specified by the internals). When you directly specify it ABORT is a
higher priority
action than in case there's a column with REPLACE error action. Thus we can
even insert
not in the empty destination table.

[-- Attachment #2: Type: text/html, Size: 3832 bytes --]

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

[Alexander Turenko]

On Fri, May 04, 2018 at 12:54:30PM +0000, Hollow111 wrote:
>    > @@ -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?
>    Yes, it is, but there's a little difference between directly specified
>    ABORT for an
>    insert stmt (INSERT OR ABORT) and just INSERT without any specified
>    error action
>    (ABORT specified by the internals). When you directly specify it ABORT
>    is a higher priority
>    action than in case there's a column with REPLACE error action. Thus we
>    can even insert
>    not in the empty destination table.

If an user asks for ABORT explicitly we should make abort, I think.

As I understood the extra variable appears due to the fact than we can
have per-column conflict clauses in CREATE TABLE and per-table clause
with INSERT OR ABORT. The latter should have precedence, I think.

I don't sure whether something (behaviour? code?) should be different
from SQLite here in light of #2963 changes. Kirill, Nikita, can you
comment, please?

WBR, Alexander Turenko.

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

[Alexander Turenko]

Hi Nikita!

Please, consider comments below.

WBR, Alexander Turenko.

> +test:do_execsql_test(
> +       "xfer-oprimization-1.8",
> ...
> +test:do_execsql_test(
> +       "xfer-oprimization-1.10",
> ...
> +test:do_execsql_test(
> +       "xfer-oprimization-1.12",
> ...
> +test:do_execsql_test(
> +       "xfer-oprimization-1.16",
> +test:do_execsql_test(
> +       "xfer-oprimization-1.18",

oprimization -> optimization

It seems that review questions from the last Nikita email in the thread
was not fixed or answered (at least some of them).

> +       sqlite3VdbeAddOp3(v, OP_OpenWrite, iSrc,
> +                         pSrc->tnum, space_ptr_reg);

Why do you open source space for write?

How your changes to xferCompatibleIndex and empty check enabling
condition is motivated? It is hard to understand it without more
detailed description.

---
EOF.

On Thu, Jun 28, 2018 at 01:18:39PM +0300, Alexander Turenko wrote:
> On Fri, May 04, 2018 at 12:54:30PM +0000, Hollow111 wrote:
> >    > @@ -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?
> >    Yes, it is, but there's a little difference between directly specified
> >    ABORT for an
> >    insert stmt (INSERT OR ABORT) and just INSERT without any specified
> >    error action
> >    (ABORT specified by the internals). When you directly specify it ABORT
> >    is a higher priority
> >    action than in case there's a column with REPLACE error action. Thus we
> >    can even insert
> >    not in the empty destination table.
> 
> If an user asks for ABORT explicitly we should make abort, I think.
> 
> As I understood the extra variable appears due to the fact than we can
> have per-column conflict clauses in CREATE TABLE and per-table clause
> with INSERT OR ABORT. The latter should have precedence, I think.
> 
> I don't sure whether something (behaviour? code?) should be different
> from SQLite here in light of #2963 changes. Kirill, Nikita, can you
> comment, please?
> 
> WBR, Alexander Turenko.
> 

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

[Nikita Tatunov]

[-- Attachment #1: Type: text/plain, Size: 3383 bytes --]

пн, 9 июл. 2018 г. в 18:50, Alexander Turenko <
alexander.turenko@tarantool.org>:

> Hi Nikita!
>
> Please, consider comments below.
>
> WBR, Alexander Turenko.
>
> > +test:do_execsql_test(
> > +       "xfer-oprimization-1.8",
> > ...
> > +test:do_execsql_test(
> > +       "xfer-oprimization-1.10",
> > ...
> > +test:do_execsql_test(
> > +       "xfer-oprimization-1.12",
> > ...
> > +test:do_execsql_test(
> > +       "xfer-oprimization-1.16",
> > +test:do_execsql_test(
> > +       "xfer-oprimization-1.18",
>
> oprimization -> optimization
>

Is already fixed.


> It seems that review questions from the last Nikita email in the thread
> was not fixed or answered (at least some of them).
>
> > +       sqlite3VdbeAddOp3(v, OP_OpenWrite, iSrc,
> > +                         pSrc->tnum, space_ptr_reg);
>
> Why do you open source space for write?
>
>
I changed it to 'vdbe_emit_open_cursor' which now is used everywhere for
both reading and writing (internally it uses 'OP_OpenWrite')

How your changes to xferCompatibleIndex and empty check enabling
> condition is motivated? It is hard to understand it without more
> detailed description.
>
>
We open cursors on PK for both tables, additional check that we have opened
'em on PK isn't redundunt.
What's about empty check enabling: I only want to add some cases where we
don't demand empty destination table for xferOptimization to be used. Thus
it will be used a little bit more frequently (it's faster than inserting
non-raw data for about 13%).


> ---
> EOF.
>
> On Thu, Jun 28, 2018 at 01:18:39PM +0300, Alexander Turenko wrote:
> > On Fri, May 04, 2018 at 12:54:30PM +0000, Hollow111 wrote:
> > >    > @@ -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?
> > >    Yes, it is, but there's a little difference between directly
> specified
> > >    ABORT for an
> > >    insert stmt (INSERT OR ABORT) and just INSERT without any specified
> > >    error action
> > >    (ABORT specified by the internals). When you directly specify it
> ABORT
> > >    is a higher priority
> > >    action than in case there's a column with REPLACE error action.
> Thus we
> > >    can even insert
> > >    not in the empty destination table.
> >
> > If an user asks for ABORT explicitly we should make abort, I think.
> >
> > As I understood the extra variable appears due to the fact than we can
> > have per-column conflict clauses in CREATE TABLE and per-table clause
> > with INSERT OR ABORT. The latter should have precedence, I think.
> >
> > I don't sure whether something (behaviour? code?) should be different
> > from SQLite here in light of #2963 changes. Kirill, Nikita, can you
> > comment, please?
> >
> > WBR, Alexander Turenko.
> >
>

[-- Attachment #2: Type: text/html, Size: 4715 bytes --]

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

[n.pettik]

Could you please rebase patch on fresh 2.0?
After that, I will give final assessment.

Also, did you manage to check that xfer optimisation really works at runtime?

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

[Nikita Tatunov]

[-- Attachment #1: Type: text/plain, Size: 398 bytes --]

пн, 16 июл. 2018 г. в 16:06, n.pettik <korablev@tarantool.org>:

> Could you please rebase patch on fresh 2.0?
> After that, I will give final assessment.
>
>
Yes, no problem.


> Also, did you manage to check that xfer optimisation really works at
> runtime?
>

Yes, I have it in newer tests and involved an int variable on this purpose.
Only need to make small fixes in it.

[-- Attachment #2: Type: text/html, Size: 809 bytes --]

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

[Nikita Tatunov]

[-- Attachment #1: Type: text/plain, Size: 768 bytes --]

Here's diff for the fixed and rebased patch. There're some unexpected
things concerned with the check of emptiness of destination table:
internals can only correctly deal with ABORT conflict action and also my
'small optimization of optimization' was incorrect.

пн, 16 июл. 2018 г. в 16:20, Nikita Tatunov <hollow653@gmail.com>:

>
> пн, 16 июл. 2018 г. в 16:06, n.pettik <korablev@tarantool.org>:
>
>> Could you please rebase patch on fresh 2.0?
>> After that, I will give final assessment.
>>
>>
> Yes, no problem.
>
>
>> Also, did you manage to check that xfer optimisation really works at
>> runtime?
>>
>
> Yes, I have it in newer tests and involved an int variable on this
> purpose. Only need to make small fixes in it.
>

[-- Attachment #2: Type: text/html, Size: 1420 bytes --]

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

[n.pettik]

> Here's diff for the fixed and rebased patch. There're some unexpected things concerned with the check of emptiness of destination table: internals can only correctly deal with ABORT conflict action and also my 'small optimization of optimization' was incorrect.

I see no diff, actually. You haven’t attached it to the letter.

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

[Nikita Tatunov]

[-- Attachment #1: Type: text/plain, Size: 24104 bytes --]

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

  if (pSelect == NULL)
  return 0; /* Must be of the form  INSERT INTO ... SELECT ... */
@@ -1744,8 +1746,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;
+ is_err_action_default = true;
+ }
  }
  assert(pSelect->pSrc); /* allocated even if there is no FROM clause */
  if (pSelect->pSrc->nSrc != 1) {
@@ -1848,9 +1852,6 @@ xferOptimization(Parse * pParse, /* Parser context */
  }
  }
  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;
@@ -1888,72 +1889,60 @@ xferOptimization(Parse * pParse, /* Parser context
*/
  * least a possibility, though it might only work if the destination
  * table (tab1) is initially empty.
  */
-#ifdef SQLITE_TEST
- sqlite3_xferopt_count++;
-#endif
+
  v = sqlite3GetVdbe(pParse);
  iSrc = pParse->nTab++;
  iDest = pParse->nTab++;
  regData = sqlite3GetTempReg(pParse);
  regTupleid = sqlite3GetTempReg(pParse);
  sqlite3OpenTable(pParse, iDest, pDest, OP_OpenWrite);
- 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.
- */
+
+ 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 = 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.
+ */
+
+ 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++;
+#endif
  }
-
- for (pDestIdx = pDest->pIndex; pDestIdx; pDestIdx = pDestIdx->pNext) {
- for (pSrcIdx = pSrc->pIndex; ALWAYS(pSrcIdx);
-      pSrcIdx = pSrcIdx->pNext) {
- if (xferCompatibleIndex(pDestIdx, pSrcIdx))
- break;
- }
- assert(pSrcIdx);
- struct space *src_space =
- space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrcIdx->tnum));
- vdbe_emit_open_cursor(pParse, iSrc,
-       SQLITE_PAGENO_TO_INDEXID(pSrcIdx->tnum),
-       src_space);
- VdbeComment((v, "%s", pSrcIdx->zName));
- struct space *dest_space =
- space_by_id(SQLITE_PAGENO_TO_SPACEID(pDestIdx->tnum));
- vdbe_emit_open_cursor(pParse, iDest,
-       SQLITE_PAGENO_TO_INDEXID(pDestIdx->tnum),
-       dest_space);
- VdbeComment((v, "%s", pDestIdx->zName));
- addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0);
- VdbeCoverage(v);
- sqlite3VdbeAddOp2(v, OP_RowData, iSrc, regData);
- sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData);
- if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY)
- 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);
+#ifdef SQLITE_TEST
+ else {
+ sql_xferOpt_count++;
  }
+#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);
+ 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
+++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
@@ -0,0 +1,706 @@
+#!/usr/bin/env tarantool
+test = require("sqltester")
+test:plan(44)
+
+local bfr, aftr
+
+test:do_catchsql_test(
+ "xfer-optimization-1.1",
+ [[
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.1>
+ 0
+ -- <xfer-optimization-1.1>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.2",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.2>
+ 1, 1, 2, 2, 3, 3
+ -- <xfer-optimization-1.2>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.3",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(id INTEGER PRIMARY KEY, b INTEGER);
+ CREATE TABLE t2(id INTEGER PRIMARY KEY, b INTEGER);
+ CREATE INDEX i1 ON t1(b);
+ CREATE INDEX i2 ON t2(b);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.3>
+ 0
+ -- <xfer-optimization-1.3>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.4",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.4>
+ 1, 1, 2, 2, 3, 3
+ -- <xfer-optimization-1.4>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.5",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER, c INTEGER);
+ INSERT INTO t1 VALUES (1, 1, 2), (2, 2, 3), (3, 3, 4);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.5>
+ 1, "table T2 has 2 columns but 3 values were supplied"
+ -- <xfer-optimization-1.5>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.6",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.6>
+
+ -- <xfer-optimization-1.6>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.7",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.7>
+ 0
+ -- <xfer-optimization-1.7>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.8",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.6>
+ 1, 1, 2, 2, 3, 3
+ -- <xfer-optimization-1.6>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.9",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 2);
+ CREATE TABLE t2(b INTEGER, a INTEGER PRIMARY KEY);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.9>
+ 1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space
'T2'"
+ -- <xfer-optimization-1.9>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.10",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.10>
+
+ -- <xfer-optimization-1.10>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.11",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 2);
+ CREATE TABLE t2(b INTEGER PRIMARY KEY, a INTEGER);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.11>
+ 0
+ -- <xfer-optimization-1.11>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.12",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.12>
+ 1, 1, 2, 2, 3, 2
+ -- <xfer-optimization-1.12>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.13",
+ [[
+ 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 (3, 3), (4, 4), (5, 5);
+ INSERT INTO t2 VALUES (1, 1), (2, 2);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.13>
+ 0
+ -- <xfer-optimization-1.13>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.14",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.14>
+ 1, 1, 2, 2, 3, 3, 4, 4, 5, 5
+ -- <xfer-optimization-1.14>
+ })
+
+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;
+ ]], {
+ -- <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}
+ 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
+-------------------------------------------------------------------------------------------
+
+bfr = box.sql.debug().sql_xferOpt_count
+
+test:do_catchsql_test(
+ "xfer-optimization-1.20",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY ON CONFLICT REPLACE, b);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY ON CONFLICT REPLACE, b);
+ CREATE TABLE t3(id INT PRIMARY KEY);
+ INSERT INTO t1 VALUES (1, 1), (3, 3), (5, 5);
+ BEGIN;
+ INSERT INTO t3 VALUES (1);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.20>
+ 0
+ -- <xfer-optimization-1.20>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.21",
+ [[
+ INSERT INTO t2 VALUES (10, 10);
+ COMMIT;
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.21>
+ 1, 1, 3, 3, 5, 5, 10, 10
+ -- <xfer-optimization-1.21>
+ })
+
+aftr = box.sql.debug().sql_xferOpt_count
+
+test:do_execsql_test(
+ "xfer-optimization-1.22",
+ [[
+ SELECT * FROM t3;
+ ]], {
+ -- <xfer-optimization-1.22>
+ 1
+ -- <xfer-optimization-1.22>
+ })
+
+test:do_test(
+ "xfer-optimization-1.23",
+ function()
+ if (aftr - bfr == 1) then
+ return {1}
+ end
+ if (aftr == bfr) then
+ return {0}
+ end
+ return {2}
+ end, {
+ -- <xfer-optimization-1.23>
+ 1
+ -- <xfer-optimization-1.23>
+ })
+
+-- 1.2) insert w/o explicit confl. action & w/ index replace action &
non-empty dest_table
+-------------------------------------------------------------------------------------------
+
+bfr = box.sql.debug().sql_xferOpt_count
+
+test:do_catchsql_test(
+ "xfer-optimization-1.24",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ DROP TABLE t3;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY ON CONFLICT REPLACE, b);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY ON CONFLICT REPLACE, 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.24>
+ 0
+ -- <xfer-optimization-1.24>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.25",
+ [[
+ INSERT INTO t2 VALUES (10, 10);
+ COMMIT;
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.25>
+ 1, 1, 2, 2, 3, 3, 4, 4, 5, 5, 10, 10
+ -- <xfer-optimization-1.25>
+ })
+
+aftr = box.sql.debug().sql_xferOpt_count
+
+test:do_test(
+ "xfer-optimization-1.26",
+ function()
+ if (aftr - bfr == 1) then
+ return {1}
+ end
+ if (aftr == bfr) then
+ return {0}
+ end
+ return {2}
+ end, {
+ -- <xfer-optimization-1.26>
+ 0
+ -- <xfer-optimization-1.26>
+ })
+
+-- 2) insert with abort
+-------------------------------------------------------------------------------------------
+
+bfr = box.sql.debug().sql_xferOpt_count
+
+test:do_catchsql_test(
+ "xfer-optimization-1.27",
+ [[
+ 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 OR ABORT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.27>
+ 1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space
'T2'"
+ -- <xfer-optimization-1.27>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.28",
+ [[
+ INSERT INTO t2 VALUES (10, 10);
+ COMMIT;
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.28>
+ 2, 2, 3, 4, 4, 4, 10, 10
+ -- <xfer-optimization-1.28>
+ })
+
+aftr = box.sql.debug().sql_xferOpt_count
+
+test:do_test(
+ "xfer-optimization-1.29",
+ function()
+ if (aftr - bfr == 1) then
+ return {1}
+ end
+ if (aftr == bfr) then
+ return {0}
+ end
+ return {2}
+ end, {
+ -- <xfer-optimization-1.29>
+ 1
+ -- <xfer-optimization-1.29>
+ })
+
+-- 3.0) insert with rollback (into empty table)
+-------------------------------------------------------------------------------------------
+
+bfr = box.sql.debug().sql_xferOpt_count
+
+test:do_catchsql_test(
+ "xfer-optimization-1.30",
+ [[
+ 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);
+ BEGIN;
+ INSERT OR ROLLBACK INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.30>
+ 0
+ -- <xfer-optimization-1.30>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.31",
+ [[
+ INSERT INTO t2 VALUES (10, 10);
+ COMMIT;
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.31>
+ 1, 1, 3, 3, 5, 5, 10, 10
+ -- <xfer-optimization-1.31>
+ })
+
+aftr = box.sql.debug().sql_xferOpt_count
+
+test:do_test(
+ "xfer-optimization-1.32",
+ function()
+ if (aftr - bfr == 1) then
+ return {1}
+ end
+ if (aftr == bfr) then
+ return {0}
+ end
+ return {2}
+ end, {
+ -- <xfer-optimization-1.32>
+ 1
+ -- <xfer-optimization-1.32>
+ })
+
+-- 3.1) insert with rollback (into non-empty table)
+-------------------------------------------------------------------------------------------
+
+bfr = box.sql.debug().sql_xferOpt_count
+
+test:do_catchsql_test(
+ "xfer-optimization-1.33",
+ [[
+ 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 OR ROLLBACK INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.33>
+ 1, "UNIQUE constraint failed: T2.A"
+ -- <xfer-optimization-1.33>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.34",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.34>
+ 2, 2, 3, 4
+ -- <xfer-optimization-1.34>
+ })
+
+aftr = box.sql.debug().sql_xferOpt_count
+
+test:do_test(
+ "xfer-optimization-1.35",
+ function()
+ if (aftr - bfr == 1) then
+ return {1}
+ end
+ if (aftr == bfr) then
+ return {0}
+ end
+ return {2}
+ end, {
+ -- <xfer-optimization-1.35>
+ 0
+ -- <xfer-optimization-1.35>
+ })
+
+-- 4) insert with replace
+-------------------------------------------------------------------------------------------
+
+bfr = box.sql.debug().sql_xferOpt_count
+
+test:do_catchsql_test(
+ "xfer-optimization-1.36",
+ [[
+ 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 OR REPLACE INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.36>
+ 0
+ -- <xfer-optimization-1.36>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.37",
+ [[
+ INSERT INTO t2 VALUES (10, 10);
+ COMMIT;
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.37>
+ 1, 1, 2, 2, 3, 3, 4, 4, 5, 5, 10, 10
+ -- <xfer-optimization-1.37>
+ })
+
+aftr = box.sql.debug().sql_xferOpt_count
+
+test:do_test(
+ "xfer-optimization-1.38",
+ function()
+ if (aftr - bfr == 1) then
+ return {1}
+ end
+ if (aftr == bfr) then
+ return {0}
+ end
+ return {2}
+ end, {
+ -- <xfer-optimization-1.38>
+ 0
+ -- <xfer-optimization-1.38>
+ })
+
+-- 5) insert with fail
+-------------------------------------------------------------------------------------------
+
+bfr = box.sql.debug().sql_xferOpt_count
+
+test:do_catchsql_test(
+ "xfer-optimization-1.39",
+ [[
+ 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 OR FAIL INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.39>
+ 1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space
'T2'"
+ -- <xfer-optimization-1.39>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.40",
+ [[
+ INSERT INTO t2 VALUES (10, 10);
+ COMMIT;
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.40>
+ 1, 1, 2, 2, 3, 4, 4, 4, 10, 10
+ -- <xfer-optimization-1.40>
+ })
+
+aftr = box.sql.debug().sql_xferOpt_count
+
+test:do_test(
+ "xfer-optimization-1.41",
+ function()
+ if (aftr - bfr == 1) then
+ return {1}
+ end
+ if (aftr == bfr) then
+ return {0}
+ end
+ return {2}
+ end, {
+ -- <xfer-optimization-1.41>
+ 0
+ -- <xfer-optimization-1.41>
+ })
+
+-- 6) insert with ignore
+-------------------------------------------------------------------------------------------
+
+bfr = box.sql.debug().sql_xferOpt_count
+
+test:do_catchsql_test(
+ "xfer-optimization-1.42",
+ [[
+ 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 OR IGNORE INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.42>
+ 0
+ -- <xfer-optimization-1.42>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.43",
+ [[
+ INSERT INTO t2 VALUES (10, 10);
+ COMMIT;
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.43>
+ 1, 1, 2, 2, 3, 4, 4, 4, 5, 5, 10, 10
+ -- <xfer-optimization-1.43>
+ })
+
+aftr = box.sql.debug().sql_xferOpt_count
+
+test:do_test(
+ "xfer-optimization-1.44",
+ function()
+ if (aftr - bfr == 1) then
+ return {1}
+ end
+ if (aftr == bfr) then
+ return {0}
+ end
+ return {2}
+ end, {
+ -- <xfer-optimization-1.44>
+ 0
+ -- <xfer-optimization-1.44>
+ })
+
+test:finish_test()


пн, 16 июл. 2018 г. в 22:12, n.pettik <korablev@tarantool.org>:

>
> > Here's diff for the fixed and rebased patch. There're some unexpected
> things concerned with the check of emptiness of destination table:
> internals can only correctly deal with ABORT conflict action and also my
> 'small optimization of optimization' was incorrect.
>
> I see no diff, actually. You haven’t attached it to the letter.

[-- Attachment #2: Type: text/html, Size: 60612 bytes --]

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

[n.pettik]

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).

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

[Nikita Tatunov]

[-- Attachment #1: Type: text/plain, Size: 36333 bytes --]

Hello, Nikita! Here's newer version of the patch.
Diff can be found at the end.

ср, 18 июл. 2018 г. в 18:13, n.pettik <korablev@tarantool.org>:

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


> > 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’.
>
>
Fixed.


> >       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?
>
>
It's all about conflict action priorities as I said before.
Consider the following example:
```
CREATE TABLE t1(a INTEGER PRIMARY KEY ON CONFLICT REPLACE, b);
CREATE TABLE t2(a INTEGER PRIMARY KEY ON CONFLICT REPLACE, 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;
INSERT INTO t2 VALUES (10, 10);
COMMIT;
```
As we understand *_REPLACE should work in this case but
onError == *_ABORT (as it was converted from the default one).
It leads to a situation where an error will occur if xferOptimization is
used.

> +     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.
>

Fixed 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.
>  * …
>  */
>

Fixed.


>
> > +
> > +     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.
>

Fixed it.


>
> > +     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.
>
>
There are basically almost the same tests surrounded by transactions (1.30
- 1.35).


> > +     ]], {
> > +             -- <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).
>

diff --git a/src/box/sql.c b/src/box/sql.c
index fdce224..656ba17 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_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_xfer_count", sql_xfer_count);
  info_end(h);
 }

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 2c9188e..c2df1c2 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1628,16 +1628,6 @@ sqlite3OpenTableAndIndices(Parse * pParse, /*
Parsing context */
  return i;
 }

-#ifdef SQLITE_TEST
-/*
- * The following global variable is incremented whenever the
- * transfer optimization is used.  This is used for testing
- * purposes only - to make sure the transfer optimization really
- * is happening when it is supposed to.
- */
-int sqlite3_xferopt_count;
-#endif /* SQLITE_TEST */
-
 #ifndef SQLITE_OMIT_XFER_OPT
 /*
  * Check to see if index pSrc is compatible as a source of data
@@ -1658,6 +1648,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 */
  }
@@ -1724,10 +1716,9 @@ xferOptimization(Parse * pParse, /* Parser context */
  int addr1; /* Loop addresses */
  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;

  if (pSelect == NULL)
  return 0; /* Must be of the form  INSERT INTO ... SELECT ... */
@@ -1744,8 +1735,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;
+ is_err_action_default = true;
+ }
  }
  assert(pSelect->pSrc); /* allocated even if there is no FROM clause */
  if (pSelect->pSrc->nSrc != 1) {
@@ -1807,6 +1800,10 @@ xferOptimization(Parse * pParse, /* Parser context */
  /* Both tables must have the same INTEGER PRIMARY KEY. */
  if (pDest->iPKey != pSrc->iPKey)
  return 0;
+ uint32_t src_space_id = SQLITE_PAGENO_TO_SPACEID(pSrc->tnum);
+ struct space *src_space = space_by_id(src_space_id);
+ uint32_t dest_space_id = SQLITE_PAGENO_TO_SPACEID(pDest->tnum);
+ struct space *dest_space = space_by_id(dest_space_id);
  for (i = 0; i < (int)pDest->def->field_count; i++) {
  enum affinity_type dest_affinity =
  pDest->def->fields[i].affinity;
@@ -1826,14 +1823,6 @@ xferOptimization(Parse * pParse, /* Parser context */
  }
  /* Default values for second and subsequent columns need to match. */
  if (i > 0) {
- uint32_t src_space_id =
- SQLITE_PAGENO_TO_SPACEID(pSrc->tnum);
- struct space *src_space =
- space_cache_find(src_space_id);
- uint32_t dest_space_id =
- SQLITE_PAGENO_TO_SPACEID(pDest->tnum);
- struct space *dest_space =
- space_cache_find(dest_space_id);
  assert(src_space != NULL && dest_space != NULL);
  char *src_expr_str =
  src_space->def->fields[i].default_value;
@@ -1848,9 +1837,6 @@ xferOptimization(Parse * pParse, /* Parser context */
  }
  }
  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;
@@ -1860,10 +1846,8 @@ xferOptimization(Parse * pParse, /* Parser context */
  }
  }
  /* Get server checks. */
- ExprList *pCheck_src = space_checks_expr_list(
- SQLITE_PAGENO_TO_SPACEID(pSrc->tnum));
- ExprList *pCheck_dest = space_checks_expr_list(
- SQLITE_PAGENO_TO_SPACEID(pDest->tnum));
+ ExprList *pCheck_src = space_checks_expr_list(src_space_id);
+ ExprList *pCheck_dest = space_checks_expr_list(dest_space_id);
  if (pCheck_dest != NULL &&
      sqlite3ExprListCompare(pCheck_src, pCheck_dest, -1) != 0) {
  /* Tables have different CHECK constraints.  Ticket #2252 */
@@ -1888,72 +1872,51 @@ xferOptimization(Parse * pParse, /* Parser context
*/
  * least a possibility, though it might only work if the destination
  * table (tab1) is initially empty.
  */
-#ifdef SQLITE_TEST
- sqlite3_xferopt_count++;
-#endif
- v = sqlite3GetVdbe(pParse);
+
+ /* The Vdbe we're building*/
+ Vdbe *v = sqlite3GetVdbe(pParse);
  iSrc = pParse->nTab++;
  iDest = pParse->nTab++;
  regData = sqlite3GetTempReg(pParse);
  regTupleid = sqlite3GetTempReg(pParse);
- sqlite3OpenTable(pParse, iDest, pDest, OP_OpenWrite);
- 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.
- */
+
+ vdbe_emit_open_cursor(pParse, iDest, 0, dest_space);
+ VdbeComment((v, "%s", pDest->def->name));
+
+ /*
+ * 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.
+ */
+
+ 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);
  }

- for (pDestIdx = pDest->pIndex; pDestIdx; pDestIdx = pDestIdx->pNext) {
- for (pSrcIdx = pSrc->pIndex; ALWAYS(pSrcIdx);
-      pSrcIdx = pSrcIdx->pNext) {
- if (xferCompatibleIndex(pDestIdx, pSrcIdx))
- break;
- }
- assert(pSrcIdx);
- struct space *src_space =
- space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrcIdx->tnum));
- vdbe_emit_open_cursor(pParse, iSrc,
-       SQLITE_PAGENO_TO_INDEXID(pSrcIdx->tnum),
-       src_space);
- VdbeComment((v, "%s", pSrcIdx->zName));
- struct space *dest_space =
- space_by_id(SQLITE_PAGENO_TO_SPACEID(pDestIdx->tnum));
- vdbe_emit_open_cursor(pParse, iDest,
-       SQLITE_PAGENO_TO_INDEXID(pDestIdx->tnum),
-       dest_space);
- VdbeComment((v, "%s", pDestIdx->zName));
- addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0);
- VdbeCoverage(v);
- sqlite3VdbeAddOp2(v, OP_RowData, iSrc, regData);
- sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData);
- if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY)
- 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);
- }
+ vdbe_emit_open_cursor(pParse, iSrc, 0, src_space);
+ VdbeComment((v, "%s", pSrc->def->name));
+ addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0);
+ VdbeCoverage(v);
+ sqlite3VdbeAddOp2(v, OP_RowData, iSrc, regData);
+
+#ifdef SQLITE_TEST
+ sqlite3VdbeChangeP5(v, OPFLAG_XFER_OPT);
+#endif
+
+ 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/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 18bf949..4b84695 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3008,6 +3008,10 @@ struct Parse {
 #define OPFLAG_NOOP_IF_NULL  0x02 /* OP_FCopy: if source register is NULL
  * then do nothing
  */
+/* OP_RowData: xferOptimization started processing */
+#ifdef SQLITE_TEST
+#define OPFLAG_XFER_OPT      0x01
+#endif

 /*
  * Each trigger present in the database schema is stored as an
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index f50e389..5f9bc13 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -77,6 +77,16 @@
 int sql_search_count = 0;
 #endif

+#ifdef SQLITE_TEST
+/*
+ * The following global variable is incremented whenever the
+ * transfer optimization is used.  This is used for testing
+ * purposes only - to make sure the transfer optimization really
+ * is happening when it is supposed to.
+ */
+int sql_xfer_count = 0;
+#endif
+
 /*
  * When this global variable is positive, it gets decremented once before
  * each instruction in the VDBE.  When it reaches zero, the
u1.isInterrupted
@@ -3976,7 +3986,7 @@ case OP_SorterData: {
  break;
 }

-/* Opcode: RowData P1 P2 * * *
+/* Opcode: RowData P1 P2 * * P5
  * Synopsis: r[P2]=data
  *
  * Write into register P2 the complete row content for the row at
@@ -3984,6 +3994,8 @@ case OP_SorterData: {
  * There is no interpretation of the data.
  * It is just copied onto the P2 register exactly as
  * it is found in the database file.
+ * P5 can be used in debug mode to check if xferOptimization has
+ * actually started processing.
  *
  * If cursor P1 is an index, then the content is the key of the row.
  * If cursor P2 is a table, then the content extracted is the data.
@@ -3996,6 +4008,13 @@ case OP_RowData: {
  BtCursor *pCrsr;
  u32 n;

+#ifdef SQLITE_TEST
+ if (pOp->p5 == 1) {
+ pOp->p5 = 0;
+ sql_xfer_count++;
+ }
+#endif
+
  pOut = &aMem[pOp->p2];
  memAboutToChange(p, pOut);

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..34f603f
--- /dev/null
+++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
@@ -0,0 +1,601 @@
+#!/usr/bin/env tarantool
+test = require("sqltester")
+test:plan(44)
+
+local bfr, aftr
+
+test:do_catchsql_test(
+ "xfer-optimization-1.1",
+ [[
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.1>
+ 0
+ -- <xfer-optimization-1.1>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.2",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.2>
+ 1, 1, 2, 2, 3, 3
+ -- <xfer-optimization-1.2>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.3",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(id INTEGER PRIMARY KEY, b INTEGER);
+ CREATE TABLE t2(id INTEGER PRIMARY KEY, b INTEGER);
+ CREATE INDEX i1 ON t1(b);
+ CREATE INDEX i2 ON t2(b);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.3>
+ 0
+ -- <xfer-optimization-1.3>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.4",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.4>
+ 1, 1, 2, 2, 3, 3
+ -- <xfer-optimization-1.4>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.5",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER, c INTEGER);
+ INSERT INTO t1 VALUES (1, 1, 2), (2, 2, 3), (3, 3, 4);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.5>
+ 1, "table T2 has 2 columns but 3 values were supplied"
+ -- <xfer-optimization-1.5>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.6",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.6>
+
+ -- <xfer-optimization-1.6>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.7",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.7>
+ 0
+ -- <xfer-optimization-1.7>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.8",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.6>
+ 1, 1, 2, 2, 3, 3
+ -- <xfer-optimization-1.6>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.9",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 2);
+ CREATE TABLE t2(b INTEGER, a INTEGER PRIMARY KEY);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.9>
+ 1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space
'T2'"
+ -- <xfer-optimization-1.9>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.10",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.10>
+
+ -- <xfer-optimization-1.10>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.11",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER);
+ INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 2);
+ CREATE TABLE t2(b INTEGER PRIMARY KEY, a INTEGER);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.11>
+ 0
+ -- <xfer-optimization-1.11>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.12",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.12>
+ 1, 1, 2, 2, 3, 2
+ -- <xfer-optimization-1.12>
+ })
+
+test:do_catchsql_test(
+ "xfer-optimization-1.13",
+ [[
+ 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 (3, 3), (4, 4), (5, 5);
+ INSERT INTO t2 VALUES (1, 1), (2, 2);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.13>
+ 0
+ -- <xfer-optimization-1.13>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.14",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.14>
+ 1, 1, 2, 2, 3, 3, 4, 4, 5, 5
+ -- <xfer-optimization-1.14>
+ })
+
+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;
+ ]], {
+ -- <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
+
+local function do_xfer_test(test_number, return_code)
+ test_name = string.format("xfer-optimization-1.%d", test_number)
+ test:do_test(
+ test_name,
+ function()
+ if (aftr - bfr == 1) then
+ return {1}
+ end
+ if (aftr == bfr) then
+ return {0}
+ end
+ end, {
+ -- <test_name>
+ return_code
+ -- <test_name>
+ })
+end
+
+-- 1.0) insert w/o explicit confl. action & w/o index replace action
+------------------------------------------------------------------------------
+
+bfr = box.sql.debug().sql_xfer_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_xfer_count
+
+do_xfer_test(19, 0)
+
+-- 1.1) insert w/o explicit confl. action & w/
+--      index replace action & empty dest_table
+------------------------------------------------------------------------------
+
+bfr = box.sql.debug().sql_xfer_count
+
+test:do_catchsql_test(
+ "xfer-optimization-1.20",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY ON CONFLICT REPLACE, b);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY ON CONFLICT REPLACE, b);
+ CREATE TABLE t3(id INT PRIMARY KEY);
+ INSERT INTO t1 VALUES (1, 1), (3, 3), (5, 5);
+ BEGIN;
+ INSERT INTO t3 VALUES (1);
+ INSERT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.20>
+ 0
+ -- <xfer-optimization-1.20>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.21",
+ [[
+ INSERT INTO t2 VALUES (10, 10);
+ COMMIT;
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.21>
+ 1, 1, 3, 3, 5, 5, 10, 10
+ -- <xfer-optimization-1.21>
+ })
+
+aftr = box.sql.debug().sql_xfer_count
+
+test:do_execsql_test(
+ "xfer-optimization-1.22",
+ [[
+ SELECT * FROM t3;
+ ]], {
+ -- <xfer-optimization-1.22>
+ 1
+ -- <xfer-optimization-1.22>
+ })
+
+do_xfer_test(23, 1)
+
+-- 1.2) insert w/o explicit confl. action & w/
+-- index replace action & non-empty dest_table
+------------------------------------------------------------------------------
+
+bfr = box.sql.debug().sql_xfer_count
+
+test:do_catchsql_test(
+ "xfer-optimization-1.24",
+ [[
+ DROP TABLE t1;
+ DROP TABLE t2;
+ DROP TABLE t3;
+ CREATE TABLE t1(a INTEGER PRIMARY KEY ON CONFLICT REPLACE, b);
+ CREATE TABLE t2(a INTEGER PRIMARY KEY ON CONFLICT REPLACE, 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.24>
+ 0
+ -- <xfer-optimization-1.24>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.25",
+ [[
+ INSERT INTO t2 VALUES (10, 10);
+ COMMIT;
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.25>
+ 1, 1, 2, 2, 3, 3, 4, 4, 5, 5, 10, 10
+ -- <xfer-optimization-1.25>
+ })
+
+aftr = box.sql.debug().sql_xfer_count
+
+do_xfer_test(26, 0)
+
+-- 2) insert with abort
+------------------------------------------------------------------------------
+
+bfr = box.sql.debug().sql_xfer_count
+
+test:do_catchsql_test(
+ "xfer-optimization-1.27",
+ [[
+ 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 OR ABORT INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.27>
+ 1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space
'T2'"
+ -- <xfer-optimization-1.27>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.28",
+ [[
+ INSERT INTO t2 VALUES (10, 10);
+ COMMIT;
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.28>
+ 2, 2, 3, 4, 4, 4, 10, 10
+ -- <xfer-optimization-1.28>
+ })
+
+aftr = box.sql.debug().sql_xfer_count
+
+do_xfer_test(29, 1)
+
+-- 3.0) insert with rollback (into empty table)
+------------------------------------------------------------------------------
+
+bfr = box.sql.debug().sql_xfer_count
+
+test:do_catchsql_test(
+ "xfer-optimization-1.30",
+ [[
+ 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);
+ BEGIN;
+ INSERT OR ROLLBACK INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.30>
+ 0
+ -- <xfer-optimization-1.30>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.31",
+ [[
+ INSERT INTO t2 VALUES (10, 10);
+ COMMIT;
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.31>
+ 1, 1, 3, 3, 5, 5, 10, 10
+ -- <xfer-optimization-1.31>
+ })
+
+aftr = box.sql.debug().sql_xfer_count
+
+do_xfer_test(32, 1)
+
+-- 3.1) insert with rollback (into non-empty table)
+------------------------------------------------------------------------------
+
+bfr = box.sql.debug().sql_xfer_count
+
+test:do_catchsql_test(
+ "xfer-optimization-1.33",
+ [[
+ 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 OR ROLLBACK INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.33>
+ 1, "UNIQUE constraint failed: T2.A"
+ -- <xfer-optimization-1.33>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.34",
+ [[
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.34>
+ 2, 2, 3, 4
+ -- <xfer-optimization-1.34>
+ })
+
+aftr = box.sql.debug().sql_xfer_count
+
+do_xfer_test(35, 0)
+
+-- 4) insert with replace
+------------------------------------------------------------------------------
+
+bfr = box.sql.debug().sql_xfer_count
+
+test:do_catchsql_test(
+ "xfer-optimization-1.36",
+ [[
+ 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 OR REPLACE INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.36>
+ 0
+ -- <xfer-optimization-1.36>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.37",
+ [[
+ INSERT INTO t2 VALUES (10, 10);
+ COMMIT;
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.37>
+ 1, 1, 2, 2, 3, 3, 4, 4, 5, 5, 10, 10
+ -- <xfer-optimization-1.37>
+ })
+
+aftr = box.sql.debug().sql_xfer_count
+
+do_xfer_test(38, 0)
+
+-- 5) insert with fail
+------------------------------------------------------------------------------
+
+bfr = box.sql.debug().sql_xfer_count
+
+test:do_catchsql_test(
+ "xfer-optimization-1.39",
+ [[
+ 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 OR FAIL INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.39>
+ 1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space
'T2'"
+ -- <xfer-optimization-1.39>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.40",
+ [[
+ INSERT INTO t2 VALUES (10, 10);
+ COMMIT;
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.40>
+ 1, 1, 2, 2, 3, 4, 4, 4, 10, 10
+ -- <xfer-optimization-1.40>
+ })
+
+aftr = box.sql.debug().sql_xfer_count
+
+do_xfer_test(41, 0)
+
+-- 6) insert with ignore
+------------------------------------------------------------------------------
+
+bfr = box.sql.debug().sql_xfer_count
+
+test:do_catchsql_test(
+ "xfer-optimization-1.42",
+ [[
+ 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 OR IGNORE INTO t2 SELECT * FROM t1;
+ ]], {
+ -- <xfer-optimization-1.42>
+ 0
+ -- <xfer-optimization-1.42>
+ })
+
+test:do_execsql_test(
+ "xfer-optimization-1.43",
+ [[
+ INSERT INTO t2 VALUES (10, 10);
+ COMMIT;
+ SELECT * FROM t2;
+ ]], {
+ -- <xfer-optimization-1.43>
+ 1, 1, 2, 2, 3, 4, 4, 4, 5, 5, 10, 10
+ -- <xfer-optimization-1.43>
+ })
+
+aftr = box.sql.debug().sql_xfer_count
+
+do_xfer_test(44, 0)
+
+test:finish_test()

[-- Attachment #2: Type: text/html, Size: 74317 bytes --]

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

[n.pettik]

When answering on review, include only chunks related to your answers.
Otherwise, letter becomes really long..

> 
> > @@ -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?
> 
> 
> It's all about conflict action priorities as I said before.
> Consider the following example:
> ```
> 		CREATE TABLE t1(a INTEGER PRIMARY KEY ON CONFLICT REPLACE, b);
> 		CREATE TABLE t2(a INTEGER PRIMARY KEY ON CONFLICT REPLACE, 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;
> 			INSERT INTO t2 VALUES (10, 10);
> 		COMMIT;

onError is an action of whole statement, not of index.
In your example onError == ABORT == DEFAULT.
Replace action of index is not involved in code you wrote.

>+	uint32_t src_space_id = SQLITE_PAGENO_TO_SPACEID(pSrc->tnum);
>+	struct space *src_space = space_by_id(src_space_id);
>+	uint32_t dest_space_id = SQLITE_PAGENO_TO_SPACEID(pDest->tnum);
>+	struct space *dest_space = space_by_id(dest_space_id);

Move here also assert:

assert(src_space != NULL && dest_space != NULL);

> > 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.
> 
> There are basically almost the same tests surrounded by transactions (1.30 - 1.35).

If so, remove redundant tests pls.

> -/* Opcode: RowData P1 P2 * * *
> +/* Opcode: RowData P1 P2 * * P5
>   * Synopsis: r[P2]=data
>   *
>   * Write into register P2 the complete row content for the row at
> @@ -3984,6 +3994,8 @@ case OP_SorterData: {
>   * There is no interpretation of the data.
>   * It is just copied onto the P2 register exactly as
>   * it is found in the database file.
> + * P5 can be used in debug mode to check if xferOptimization has
> + * actually started processing.
>   *
>   * If cursor P1 is an index, then the content is the key of the row.
>   * If cursor P2 is a table, then the content extracted is the data.
> @@ -3996,6 +4008,13 @@ case OP_RowData: {
>  	BtCursor *pCrsr;
>  	u32 n;
>  
> +#ifdef SQLITE_TEST
> +	if (pOp->p5 == 1) {

Use named value (i.e. OPFLAG_XFER_OPT) even if they are the same.

> +
> +test:do_execsql_test(
> +	"xfer-optimization-1.37",
> +	[[
> +			INSERT INTO t2 VALUES (10, 10);

Here (and in some other places) smth wrong with indentation, fix it pls.

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

[Nikita Tatunov]

[-- Attachment #1: Type: text/plain, Size: 23587 bytes --]

чт, 19 июл. 2018 г. в 3:20, n.pettik <korablev@tarantool.org>:

> When answering on review, include only chunks related to your answers.
> Otherwise, letter becomes really long..
>
>
I'm sorry.


> >
> > > @@ -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?
> >
> >
> > It's all about conflict action priorities as I said before.
> > Consider the following example:
> > ```
> >               CREATE TABLE t1(a INTEGER PRIMARY KEY ON CONFLICT REPLACE,
> b);
> >               CREATE TABLE t2(a INTEGER PRIMARY KEY ON CONFLICT REPLACE,
> 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;
> >                       INSERT INTO t2 VALUES (10, 10);
> >               COMMIT;
>
> onError is an action of whole statement, not of index.
> In your example onError == ABORT == DEFAULT.
> Replace action of index is not involved in code you wrote.
>

As we discussed, was *_ABORT explicitly set or not makes sense
till we have conflict actions on indices.


>
> >+      uint32_t src_space_id = SQLITE_PAGENO_TO_SPACEID(pSrc->tnum);
> >+      struct space *src_space = space_by_id(src_space_id);
> >+      uint32_t dest_space_id = SQLITE_PAGENO_TO_SPACEID(pDest->tnum);
> >+      struct space *dest_space = space_by_id(dest_space_id);
>
> Move here also assert:
>
> assert(src_space != NULL && dest_space != NULL);
>
>
Done.


> > > 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.
> >
> > There are basically almost the same tests surrounded by transactions
> (1.30 - 1.35).
>
> If so, remove redundant tests pls.
>
>
Removed them. Also added tests that check if xfer
was actually used in first bunch of tests.


> > -/* Opcode: RowData P1 P2 * * *
> > +/* Opcode: RowData P1 P2 * * P5
> >   * Synopsis: r[P2]=data
> >   *
> >   * Write into register P2 the complete row content for the row at
> > @@ -3984,6 +3994,8 @@ case OP_SorterData: {
> >   * There is no interpretation of the data.
> >   * It is just copied onto the P2 register exactly as
> >   * it is found in the database file.
> > + * P5 can be used in debug mode to check if xferOptimization has
> > + * actually started processing.
> >   *
> >   * If cursor P1 is an index, then the content is the key of the row.
> >   * If cursor P2 is a table, then the content extracted is the data.
> > @@ -3996,6 +4008,13 @@ case OP_RowData: {
> >       BtCursor *pCrsr;
> >       u32 n;
> >
> > +#ifdef SQLITE_TEST
> > +     if (pOp->p5 == 1) {
>
> Use named value (i.e. OPFLAG_XFER_OPT) even if they are the same.
>
>
Thank you, forgot about this place when defined it.


> > +
> > +test:do_execsql_test(
> > +     "xfer-optimization-1.37",
> > +     [[
> > +                     INSERT INTO t2 VALUES (10, 10);
>
> Here (and in some other places) smth wrong with indentation, fix it pls.
>
>
Indentation shows if stmt is in transaction, probably it looks a little
weird,
but I'm more inclined to leave it there.

Diff that compares patch to prev. one:

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index c2df1c2..3c3bf37 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1733,12 +1733,8 @@ xferOptimization(Parse * pParse, /* Parser context */
  if (space_trigger_list(pDest->def->id) != NULL)
  return 0;
  if (onError == ON_CONFLICT_ACTION_DEFAULT) {
- if (pDest->iPKey >= 0)
- onError = pDest->keyConf;
- if (onError == ON_CONFLICT_ACTION_DEFAULT) {
- onError = ON_CONFLICT_ACTION_ABORT;
- is_err_action_default = true;
- }
+ onError = ON_CONFLICT_ACTION_ABORT;
+ is_err_action_default = true;
  }
  assert(pSelect->pSrc); /* allocated even if there is no FROM clause */
  if (pSelect->pSrc->nSrc != 1) {
@@ -1804,6 +1800,7 @@ xferOptimization(Parse * pParse, /* Parser context */
  struct space *src_space = space_by_id(src_space_id);
  uint32_t dest_space_id = SQLITE_PAGENO_TO_SPACEID(pDest->tnum);
  struct space *dest_space = space_by_id(dest_space_id);
+ assert(src_space != NULL && dest_space != NULL);
  for (i = 0; i < (int)pDest->def->field_count; i++) {
  enum affinity_type dest_affinity =
  pDest->def->fields[i].affinity;
@@ -1823,7 +1820,6 @@ xferOptimization(Parse * pParse, /* Parser context */
  }
  /* Default values for second and subsequent columns need to match. */
  if (i > 0) {
- assert(src_space != NULL && dest_space != NULL);
  char *src_expr_str =
  src_space->def->fields[i].default_value;
  char *dest_expr_str =
@@ -1885,12 +1881,11 @@ xferOptimization(Parse * pParse, /* Parser context
*/

  /*
  * 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.
+ * in case there's a conflict action other than
+ * explicit *_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.
  */
-
  if (!(onError == ON_CONFLICT_ACTION_ABORT &&
      is_err_action_default == false)) {
  addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 5f9bc13..bc169d9 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4009,7 +4009,7 @@ case OP_RowData: {
  u32 n;

 #ifdef SQLITE_TEST
- if (pOp->p5 == 1) {
+ if (pOp->p5 == OPFLAG_XFER_OPT) {
  pOp->p5 = 0;
  sql_xfer_count++;
  }
diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
index 34f603f..29f0efe 100755
--- a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
+++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
@@ -1,9 +1,29 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(44)
+test:plan(46)

 local bfr, aftr

+local function do_xfer_test(test_number, return_code)
+ test_name = string.format("xfer-optimization-1.%d", test_number)
+ test:do_test(
+ test_name,
+ function()
+ if (aftr - bfr == 1) then
+ return {1}
+ end
+ if (aftr == bfr) then
+ return {0}
+ end
+ end, {
+ -- <test_name>
+ return_code
+ -- <test_name>
+ })
+end
+
+bfr = box.sql.debug().sql_xfer_count
+
 test:do_catchsql_test(
  "xfer-optimization-1.1",
  [[
@@ -17,6 +37,8 @@ test:do_catchsql_test(
  -- <xfer-optimization-1.1>
  })

+aftr = box.sql.debug().sql_xfer_count
+
 test:do_execsql_test(
  "xfer-optimization-1.2",
  [[
@@ -27,8 +49,12 @@ test:do_execsql_test(
  -- <xfer-optimization-1.2>
  })

+do_xfer_test(3, 1)
+
+bfr = box.sql.debug().sql_xfer_count
+
 test:do_catchsql_test(
- "xfer-optimization-1.3",
+ "xfer-optimization-1.4",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -39,23 +65,29 @@ test:do_catchsql_test(
  INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
  INSERT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.3>
+ -- <xfer-optimization-1.4>
  0
- -- <xfer-optimization-1.3>
+ -- <xfer-optimization-1.4>
  })

+aftr = box.sql.debug().sql_xfer_count
+
 test:do_execsql_test(
- "xfer-optimization-1.4",
+ "xfer-optimization-1.5",
  [[
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.4>
+ -- <xfer-optimization-1.5>
  1, 1, 2, 2, 3, 3
- -- <xfer-optimization-1.4>
+ -- <xfer-optimization-1.5>
  })

+do_xfer_test(6, 1)
+
+bfr = box.sql.debug().sql_xfer_count
+
 test:do_catchsql_test(
- "xfer-optimization-1.5",
+ "xfer-optimization-1.7",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -64,23 +96,29 @@ test:do_catchsql_test(
  CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER);
  INSERT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.5>
+ -- <xfer-optimization-1.7>
  1, "table T2 has 2 columns but 3 values were supplied"
- -- <xfer-optimization-1.5>
+ -- <xfer-optimization-1.7>
  })

+aftr = box.sql.debug().sql_xfer_count
+
 test:do_execsql_test(
- "xfer-optimization-1.6",
+ "xfer-optimization-1.8",
  [[
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.6>
+ -- <xfer-optimization-1.8>

- -- <xfer-optimization-1.6>
+ -- <xfer-optimization-1.8>
  })

+do_xfer_test(9, 0)
+
+bfr = box.sql.debug().sql_xfer_count
+
 test:do_catchsql_test(
- "xfer-optimization-1.7",
+ "xfer-optimization-1.10",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -89,23 +127,29 @@ test:do_catchsql_test(
  CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER);
  INSERT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.7>
+ -- <xfer-optimization-1.10>
  0
- -- <xfer-optimization-1.7>
+ -- <xfer-optimization-1.10>
  })

+aftr = box.sql.debug().sql_xfer_count
+
 test:do_execsql_test(
- "xfer-optimization-1.8",
+ "xfer-optimization-1.11",
  [[
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.6>
+ -- <xfer-optimization-1.11>
  1, 1, 2, 2, 3, 3
- -- <xfer-optimization-1.6>
+ -- <xfer-optimization-1.11>
  })

+do_xfer_test(12, 1);
+
+bfr = box.sql.debug().sql_xfer_count
+
 test:do_catchsql_test(
- "xfer-optimization-1.9",
+ "xfer-optimization-1.13",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -114,23 +158,29 @@ test:do_catchsql_test(
  CREATE TABLE t2(b INTEGER, a INTEGER PRIMARY KEY);
  INSERT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.9>
+ -- <xfer-optimization-1.13>
  1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space
'T2'"
- -- <xfer-optimization-1.9>
+ -- <xfer-optimization-1.13>
  })

+aftr = box.sql.debug().sql_xfer_count
+
 test:do_execsql_test(
- "xfer-optimization-1.10",
+ "xfer-optimization-1.14",
  [[
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.10>
+ -- <xfer-optimization-1.14>

- -- <xfer-optimization-1.10>
+ -- <xfer-optimization-1.14>
  })

+do_xfer_test(15, 0)
+
+bfr = box.sql.debug().sql_xfer_count
+
 test:do_catchsql_test(
- "xfer-optimization-1.11",
+ "xfer-optimization-1.16",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -139,72 +189,24 @@ test:do_catchsql_test(
  CREATE TABLE t2(b INTEGER PRIMARY KEY, a INTEGER);
  INSERT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.11>
+ -- <xfer-optimization-1.16>
  0
- -- <xfer-optimization-1.11>
- })
-
-test:do_execsql_test(
- "xfer-optimization-1.12",
- [[
- SELECT * FROM t2;
- ]], {
- -- <xfer-optimization-1.12>
- 1, 1, 2, 2, 3, 2
- -- <xfer-optimization-1.12>
+ -- <xfer-optimization-1.16>
  })

-test:do_catchsql_test(
- "xfer-optimization-1.13",
- [[
- 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 (3, 3), (4, 4), (5, 5);
- INSERT INTO t2 VALUES (1, 1), (2, 2);
- INSERT INTO t2 SELECT * FROM t1;
- ]], {
- -- <xfer-optimization-1.13>
- 0
- -- <xfer-optimization-1.13>
- })
+aftr = box.sql.debug().sql_xfer_count

 test:do_execsql_test(
- "xfer-optimization-1.14",
+ "xfer-optimization-1.17",
  [[
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.14>
- 1, 1, 2, 2, 3, 3, 4, 4, 5, 5
- -- <xfer-optimization-1.14>
+ -- <xfer-optimization-1.17>
+ 1, 1, 2, 2, 3, 2
+ -- <xfer-optimization-1.17>
  })

-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;
- ]], {
- -- <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>
- })
+do_xfer_test(18, 1)

 -- The following tests are supposed to test if xfer-optimization is
actually
 -- used in the given cases (if the conflict actually occurs):
@@ -220,23 +222,6 @@ test:do_execsql_test(
 -- 5) insert with fail
 -- 6) insert with ignore

-local function do_xfer_test(test_number, return_code)
- test_name = string.format("xfer-optimization-1.%d", test_number)
- test:do_test(
- test_name,
- function()
- if (aftr - bfr == 1) then
- return {1}
- end
- if (aftr == bfr) then
- return {0}
- end
- end, {
- -- <test_name>
- return_code
- -- <test_name>
- })
-end

 -- 1.0) insert w/o explicit confl. action & w/o index replace action
 ------------------------------------------------------------------------------
@@ -244,7 +229,7 @@ end
 bfr = box.sql.debug().sql_xfer_count

 test:do_catchsql_test(
- "xfer-optimization-1.17",
+ "xfer-optimization-1.19",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -256,26 +241,26 @@ test:do_catchsql_test(
  INSERT INTO t2 VALUES (4, 4);
  INSERT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.17>
+ -- <xfer-optimization-1.19>
  1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space
'T2'"
- -- <xfer-optimization-1.17>
+ -- <xfer-optimization-1.19>
  })

 test:do_execsql_test(
- "xfer-optimization-1.18",
+ "xfer-optimization-1.20",
  [[
  INSERT INTO t2 VALUES (10, 10);
  COMMIT;
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.18>
+ -- <xfer-optimization-1.20>
  2, 2, 3, 4, 4, 4, 10, 10
- -- <xfer-optimization-1.18>
+ -- <xfer-optimization-1.20>
  })

 aftr = box.sql.debug().sql_xfer_count

-do_xfer_test(19, 0)
+do_xfer_test(21, 0)

 -- 1.1) insert w/o explicit confl. action & w/
 --      index replace action & empty dest_table
@@ -284,7 +269,7 @@ do_xfer_test(19, 0)
 bfr = box.sql.debug().sql_xfer_count

 test:do_catchsql_test(
- "xfer-optimization-1.20",
+ "xfer-optimization-1.22",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -296,36 +281,36 @@ test:do_catchsql_test(
  INSERT INTO t3 VALUES (1);
  INSERT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.20>
+ -- <xfer-optimization-1.22>
  0
- -- <xfer-optimization-1.20>
+ -- <xfer-optimization-1.22>
  })

 test:do_execsql_test(
- "xfer-optimization-1.21",
+ "xfer-optimization-1.23",
  [[
  INSERT INTO t2 VALUES (10, 10);
  COMMIT;
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.21>
+ -- <xfer-optimization-1.23>
  1, 1, 3, 3, 5, 5, 10, 10
- -- <xfer-optimization-1.21>
+ -- <xfer-optimization-1.23>
  })

 aftr = box.sql.debug().sql_xfer_count

 test:do_execsql_test(
- "xfer-optimization-1.22",
+ "xfer-optimization-1.24",
  [[
  SELECT * FROM t3;
  ]], {
- -- <xfer-optimization-1.22>
+ -- <xfer-optimization-1.24>
  1
- -- <xfer-optimization-1.22>
+ -- <xfer-optimization-1.24>
  })

-do_xfer_test(23, 1)
+do_xfer_test(25, 1)

 -- 1.2) insert w/o explicit confl. action & w/
 -- index replace action & non-empty dest_table
@@ -334,7 +319,7 @@ do_xfer_test(23, 1)
 bfr = box.sql.debug().sql_xfer_count

 test:do_catchsql_test(
- "xfer-optimization-1.24",
+ "xfer-optimization-1.26",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -347,26 +332,26 @@ test:do_catchsql_test(
  INSERT INTO t2 VALUES (4, 4);
  INSERT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.24>
+ -- <xfer-optimization-1.26>
  0
- -- <xfer-optimization-1.24>
+ -- <xfer-optimization-1.26>
  })

 test:do_execsql_test(
- "xfer-optimization-1.25",
+ "xfer-optimization-1.27",
  [[
  INSERT INTO t2 VALUES (10, 10);
  COMMIT;
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.25>
+ -- <xfer-optimization-1.27>
  1, 1, 2, 2, 3, 3, 4, 4, 5, 5, 10, 10
- -- <xfer-optimization-1.25>
+ -- <xfer-optimization-1.27>
  })

 aftr = box.sql.debug().sql_xfer_count

-do_xfer_test(26, 0)
+do_xfer_test(28, 0)

 -- 2) insert with abort
 ------------------------------------------------------------------------------
@@ -374,7 +359,7 @@ do_xfer_test(26, 0)
 bfr = box.sql.debug().sql_xfer_count

 test:do_catchsql_test(
- "xfer-optimization-1.27",
+ "xfer-optimization-1.29",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -386,26 +371,26 @@ test:do_catchsql_test(
  INSERT INTO t2 VALUES (4, 4);
  INSERT OR ABORT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.27>
+ -- <xfer-optimization-1.29>
  1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space
'T2'"
- -- <xfer-optimization-1.27>
+ -- <xfer-optimization-1.29>
  })

 test:do_execsql_test(
- "xfer-optimization-1.28",
+ "xfer-optimization-1.30",
  [[
  INSERT INTO t2 VALUES (10, 10);
  COMMIT;
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.28>
+ -- <xfer-optimization-1.30>
  2, 2, 3, 4, 4, 4, 10, 10
- -- <xfer-optimization-1.28>
+ -- <xfer-optimization-1.30>
  })

 aftr = box.sql.debug().sql_xfer_count

-do_xfer_test(29, 1)
+do_xfer_test(31, 1)

 -- 3.0) insert with rollback (into empty table)
 ------------------------------------------------------------------------------
@@ -413,7 +398,7 @@ do_xfer_test(29, 1)
 bfr = box.sql.debug().sql_xfer_count

 test:do_catchsql_test(
- "xfer-optimization-1.30",
+ "xfer-optimization-1.32",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -423,26 +408,26 @@ test:do_catchsql_test(
  BEGIN;
  INSERT OR ROLLBACK INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.30>
+ -- <xfer-optimization-1.32>
  0
- -- <xfer-optimization-1.30>
+ -- <xfer-optimization-1.32>
  })

 test:do_execsql_test(
- "xfer-optimization-1.31",
+ "xfer-optimization-1.33",
  [[
  INSERT INTO t2 VALUES (10, 10);
  COMMIT;
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.31>
+ -- <xfer-optimization-1.33>
  1, 1, 3, 3, 5, 5, 10, 10
- -- <xfer-optimization-1.31>
+ -- <xfer-optimization-1.33>
  })

 aftr = box.sql.debug().sql_xfer_count

-do_xfer_test(32, 1)
+do_xfer_test(34, 1)

 -- 3.1) insert with rollback (into non-empty table)
 ------------------------------------------------------------------------------
@@ -450,7 +435,7 @@ do_xfer_test(32, 1)
 bfr = box.sql.debug().sql_xfer_count

 test:do_catchsql_test(
- "xfer-optimization-1.33",
+ "xfer-optimization-1.35",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -462,24 +447,24 @@ test:do_catchsql_test(
  INSERT INTO t2 VALUES (4, 4);
  INSERT OR ROLLBACK INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.33>
+ -- <xfer-optimization-1.35>
  1, "UNIQUE constraint failed: T2.A"
- -- <xfer-optimization-1.33>
+ -- <xfer-optimization-1.35>
  })

 test:do_execsql_test(
- "xfer-optimization-1.34",
+ "xfer-optimization-1.36",
  [[
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.34>
+ -- <xfer-optimization-1.36>
  2, 2, 3, 4
- -- <xfer-optimization-1.34>
+ -- <xfer-optimization-1.36>
  })

 aftr = box.sql.debug().sql_xfer_count

-do_xfer_test(35, 0)
+do_xfer_test(37, 0)

 -- 4) insert with replace
 ------------------------------------------------------------------------------
@@ -487,7 +472,7 @@ do_xfer_test(35, 0)
 bfr = box.sql.debug().sql_xfer_count

 test:do_catchsql_test(
- "xfer-optimization-1.36",
+ "xfer-optimization-1.38",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -499,26 +484,26 @@ test:do_catchsql_test(
  INSERT INTO t2 VALUES (4, 4);
  INSERT OR REPLACE INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.36>
+ -- <xfer-optimization-1.38>
  0
- -- <xfer-optimization-1.36>
+ -- <xfer-optimization-1.38>
  })

 test:do_execsql_test(
- "xfer-optimization-1.37",
+ "xfer-optimization-1.39",
  [[
  INSERT INTO t2 VALUES (10, 10);
  COMMIT;
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.37>
+ -- <xfer-optimization-1.39>
  1, 1, 2, 2, 3, 3, 4, 4, 5, 5, 10, 10
- -- <xfer-optimization-1.37>
+ -- <xfer-optimization-1.39>
  })

 aftr = box.sql.debug().sql_xfer_count

-do_xfer_test(38, 0)
+do_xfer_test(40, 0)

 -- 5) insert with fail
 ------------------------------------------------------------------------------
@@ -526,7 +511,7 @@ do_xfer_test(38, 0)
 bfr = box.sql.debug().sql_xfer_count

 test:do_catchsql_test(
- "xfer-optimization-1.39",
+ "xfer-optimization-1.41",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -538,26 +523,26 @@ test:do_catchsql_test(
  INSERT INTO t2 VALUES (4, 4);
  INSERT OR FAIL INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.39>
+ -- <xfer-optimization-1.41>
  1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space
'T2'"
- -- <xfer-optimization-1.39>
+ -- <xfer-optimization-1.41>
  })

 test:do_execsql_test(
- "xfer-optimization-1.40",
+ "xfer-optimization-1.42",
  [[
  INSERT INTO t2 VALUES (10, 10);
  COMMIT;
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.40>
+ -- <xfer-optimization-1.42>
  1, 1, 2, 2, 3, 4, 4, 4, 10, 10
- -- <xfer-optimization-1.40>
+ -- <xfer-optimization-1.42>
  })

 aftr = box.sql.debug().sql_xfer_count

-do_xfer_test(41, 0)
+do_xfer_test(43, 0)

 -- 6) insert with ignore
 ------------------------------------------------------------------------------
@@ -565,7 +550,7 @@ do_xfer_test(41, 0)
 bfr = box.sql.debug().sql_xfer_count

 test:do_catchsql_test(
- "xfer-optimization-1.42",
+ "xfer-optimization-1.44",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -577,25 +562,25 @@ test:do_catchsql_test(
  INSERT INTO t2 VALUES (4, 4);
  INSERT OR IGNORE INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.42>
+ -- <xfer-optimization-1.44>
  0
- -- <xfer-optimization-1.42>
+ -- <xfer-optimization-1.44>
  })

 test:do_execsql_test(
- "xfer-optimization-1.43",
+ "xfer-optimization-1.45",
  [[
  INSERT INTO t2 VALUES (10, 10);
  COMMIT;
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.43>
+ -- <xfer-optimization-1.45>
  1, 1, 2, 2, 3, 4, 4, 4, 5, 5, 10, 10
- -- <xfer-optimization-1.43>
+ -- <xfer-optimization-1.45>
  })

 aftr = box.sql.debug().sql_xfer_count

-do_xfer_test(44, 0)
+do_xfer_test(46, 0)

 test:finish_test()

[-- Attachment #2: Type: text/html, Size: 53814 bytes --]

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

[n.pettik]

Several minor remarks.

> @@ -1885,12 +1881,11 @@ xferOptimization(Parse * pParse,	/* Parser context */
>  
>  	/*
>  	 * 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.
> +	 * in case there's a conflict action other than
> +	 * explicit *_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.
>  	 */
> -
>  	if (!(onError == ON_CONFLICT_ACTION_ABORT &&
>  	    is_err_action_default == false)) {

AFAIK we don’t use == false comparison. Instead simply use ! is_rr_action_default.

>  		addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 5f9bc13..bc169d9 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4009,7 +4009,7 @@ case OP_RowData: {
>  	u32 n;
>  
>  #ifdef SQLITE_TEST
> -	if (pOp->p5 == 1) {
> +	if (pOp->p5 == OPFLAG_XFER_OPT) {

Flags are usually tested with & operation:

if (pOp->p5 & OPFLAG_XFER_OPT != 0)

>  		pOp->p5 = 0;
>  		sql_xfer_count++;
>  	}
> diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
> index 34f603f..29f0efe 100755
> --- a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
> +++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
> @@ -1,9 +1,29 @@
>  
> +local function do_xfer_test(test_number, return_code)
> +	test_name = string.format("xfer-optimization-1.%d", test_number)
> +	test:do_test(
> +		test_name,
> +		function()
> +			if (aftr - bfr == 1) then
> +				return {1}
> +			end
> +			if (aftr == bfr) then
> +				return {0}
> +			end

You can simplify it to:

return aftr - bfr;

:)

Also

>+	/* The Vdbe we're building*/
>+	Vdbe *v = sqlite3GetVdbe(pParse);

Use ’struct’ modifier for complex types.
And put dot at the end of comment.

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

[Nikita Tatunov]

[-- Attachment #1: Type: text/plain, Size: 4392 bytes --]

Hello, Nikita! Thank you for review!

пт, 20 июл. 2018 г. в 6:20, n.pettik <korablev@tarantool.org>:

> Several minor remarks.
>
> > @@ -1885,12 +1881,11 @@ xferOptimization(Parse * pParse,      /* Parser
> context */
> >
> >       /*
> >        * 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.
> > +      * in case there's a conflict action other than
> > +      * explicit *_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.
> >        */
> > -
> >       if (!(onError == ON_CONFLICT_ACTION_ABORT &&
> >           is_err_action_default == false)) {
>
> AFAIK we don’t use == false comparison. Instead simply use !
> is_rr_action_default.
>
>
Done.


> >               addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);
> > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> > index 5f9bc13..bc169d9 100644
> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -4009,7 +4009,7 @@ case OP_RowData: {
> >       u32 n;
> >
> >  #ifdef SQLITE_TEST
> > -     if (pOp->p5 == 1) {
> > +     if (pOp->p5 == OPFLAG_XFER_OPT) {
>
> Flags are usually tested with & operation:
>
> if (pOp->p5 & OPFLAG_XFER_OPT != 0)
>
>
Fixed it.


> >               pOp->p5 = 0;
> >               sql_xfer_count++;
> >       }
> > diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
> b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
> > index 34f603f..29f0efe 100755
> > --- a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
> > +++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
> > @@ -1,9 +1,29 @@
> >
> > +local function do_xfer_test(test_number, return_code)
> > +     test_name = string.format("xfer-optimization-1.%d", test_number)
> > +     test:do_test(
> > +             test_name,
> > +             function()
> > +                     if (aftr - bfr == 1) then
> > +                             return {1}
> > +                     end
> > +                     if (aftr == bfr) then
> > +                             return {0}
> > +                     end
>
> You can simplify it to:
>
> return aftr - bfr;
>
> :)
>
>
True. Fixed it.


> Also
>
> >+      /* The Vdbe we're building*/
> >+      Vdbe *v = sqlite3GetVdbe(pParse);
>
> Use ’struct’ modifier for complex types.
> And put dot at the end of comment.
>
>
Fixed.

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 3c3bf37..4f52fa5 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1869,7 +1869,7 @@ xferOptimization(Parse * pParse, /* Parser context */
  * table (tab1) is initially empty.
  */

- /* The Vdbe we're building*/
+ /* The Vdbe struct we're building. */
  Vdbe *v = sqlite3GetVdbe(pParse);
  iSrc = pParse->nTab++;
  iDest = pParse->nTab++;
@@ -1887,7 +1887,7 @@ xferOptimization(Parse * pParse, /* Parser context */
  * That block generates code to make that determination.
  */
  if (!(onError == ON_CONFLICT_ACTION_ABORT &&
-     is_err_action_default == false)) {
+     !is_err_action_default)) {
  addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);
  VdbeCoverage(v);
  emptyDestTest = sqlite3VdbeAddOp0(v, OP_Goto);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index bc169d9..aa7f250 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4009,7 +4009,7 @@ case OP_RowData: {
  u32 n;

 #ifdef SQLITE_TEST
- if (pOp->p5 == OPFLAG_XFER_OPT) {
+ if ((pOp->p5 & OPFLAG_XFER_OPT) != 0) {
  pOp->p5 = 0;
  sql_xfer_count++;
  }
diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
index 29f0efe..05d30c6 100755
--- a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
+++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
@@ -9,12 +9,7 @@ local function do_xfer_test(test_number, return_code)
  test:do_test(
  test_name,
  function()
- if (aftr - bfr == 1) then
- return {1}
- end
- if (aftr == bfr) then
- return {0}
- end
+ return {aftr - bfr}
  end, {
  -- <test_name>
  return_code

[-- Attachment #2: Type: text/html, Size: 7657 bytes --]

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

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 482 bytes --]

LGTM.

> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 3c3bf37..4f52fa5 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -1869,7 +1869,7 @@ xferOptimization(Parse * pParse,	/* Parser context */
>  	 * table (tab1) is initially empty.
>  	 */
>  
> -	/* The Vdbe we're building*/
> +	/* The Vdbe struct we're building. */

You misunderstood me. What I mean is:

struct Vibe *v = …;

>  	Vdbe *v = sqlite3GetVdbe(pParse);


[-- Attachment #2: Type: text/html, Size: 2185 bytes --]

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

[Nikita Tatunov]

[-- Attachment #1: Type: text/plain, Size: 619 bytes --]

Ooops. Thank you! fixed it and pushed.

пт, 20 июл. 2018 г. в 19:43, n.pettik <korablev@tarantool.org>:

> LGTM.
>
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 3c3bf37..4f52fa5 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -1869,7 +1869,7 @@ xferOptimization(Parse * pParse, /* Parser context
> */
>    * table (tab1) is initially empty.
>    */
>
> - /* The Vdbe we're building*/
> + /* The Vdbe struct we're building. */
>
>
> You misunderstood me. What I mean is:
>
> struct Vibe *v = …;
>
>   Vdbe *v = sqlite3GetVdbe(pParse);
>
>
>

[-- Attachment #2: Type: text/html, Size: 2057 bytes --]

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

[Alexander Turenko]

Hi!

Please consider my comments and questions below.

WBR, Alexander Turenko.

> +       /*
> +        * Xfer optimization is unable to correctly insert data
> +        * in case there's a conflict action other than
> +        * explicit *_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.
> +        */
> +       if (!(onError == ON_CONFLICT_ACTION_ABORT &&
> +           !is_err_action_default)) {

Do you mean that:

1. The optimization non-empty table case correctly works only with ABORT
   conflict action (default or explicit).
2. The default conflict action can be overwritten on per-column basis, so
   'default abort' can really be replace or other conflict action.

If so, your description doesn't give this information.

To more on that, can we check per-column conflict actions instead of check
whether the conflict action default or explicit? This would enable more cases
with non-empty tables for the optimization. And this would look less confusing,
IMHO.

I have one more question on that. It seems that SQLite has this optimization
working with ROLLBACK conflict action. We cannot doing so, because of some
problem? Did this problem described / trackerized somewhere? Or something
changes underhood and makes this impossible? Are we know exact reason or just
observing it does not work?

> +#ifdef SQLITE_TEST
> +/*
> + * The following global variable is incremented whenever the
> + * transfer optimization is used.  This is used for testing
> + * purposes only - to make sure the transfer optimization really
> + * is happening when it is supposed to.
> + */
> +int sql_xfer_count = 0;
> +#endif

I think it would be good to mention the opcode where the counter is
incremented. You can follow the style in which other counters are
described (they are mostly mention opcodes).

> -/* Opcode: RowData P1 P2 * * *
> +/* Opcode: RowData P1 P2 * * P5

We can increase the counter on per-operation basis instead of per-row by
adding the flag to OP_OpenWrite. It will save some CPU cycles :)

> +#ifdef SQLITE_TEST
> +       if ((pOp->p5 & OPFLAG_XFER_OPT) != 0) {
> +               pOp->p5 = 0;
> +               sql_xfer_count++;
> +       }
> +#endif

1. Not actual due to 2, but it would be better to use
   `pOp->p5 &= ~OPFLAG_XFER_OPT` to drop just that flag.
2. It is counter-intuitive, IMHO, to change operation flags during that
   operation. So, said above, vote to move it to OP_OpenWrite.

> +local bfr, aftr
> +

What do you plan to do with saved letters? :) Really, such abbreviations
just makes reading harder with no gains.

> +local function do_xfer_test(test_number, return_code)
> +       test_name = string.format("xfer-optimization-1.%d", test_number)
> +       test:do_test(
> +               test_name,
> +               function()
> +                       return {aftr - bfr}
> +               end, {
> +                       -- <test_name>
> +                       return_code
> +                       -- <test_name>
> +               })
> +end

That code can be written simpler (consider tap module documentation):

test:is(after - before, exp, test_name)

I suggest to create wrappers like so (I didn't test it):

local function do_xfer_test(test_func, test, test_name, func, exp, opts)
    local opts = opts or {}
    local exp_xfer_count = opts.exp_xfer_count
    local before = box.sql.debug().sql_xfer_count
    local ok = test_func(test, test_name, func, exp)
    local after = box.sql.debug().sql_xfer_count
    if exp_xfer_count ~= nil then
        ok = ok and test:is(after - before, exp_xfer_count, test_name ..
            '_xfer_count')
    end
    return ok
end

test.do_execsql_xfer_test = function(test, test_name, func, exp, opts)
    return do_xfer_test(test.do_execsql_test, test, test_name, func, exp, opts)
end

test.do_catchsql_xfer_test = function(test, test_name, func, exp, opts)
    return do_xfer_test(test.do_catchsql_test, test, test_name, func, exp, opts)
end

And use it like so:

test:do_catchsql_xfer_test(
    "xfer-optimization-1.1",
    [[
        CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
        INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
        CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
        INSERT INTO t2 SELECT * FROM t1;
    ]], {
        -- <xfer-optimization-1.1>
        0
        -- <xfer-optimization-1.1>
    }), {
        exp_xfer_count = 1
    }
)

By the way, you can revive xfer cases in test/sql-tap/with2.test.lua. Or drop
it if your new test includes all related cases from with2.

On Fri, Jul 20, 2018 at 07:58:48PM +0300, Nikita Tatunov wrote:
>    Ooops. Thank you! fixed it and pushed.
> 
>    пт, 20 июл. 2018 г. в 19:43, n.pettik <[1]korablev@tarantool.org>:
> 
>    LGTM.
> 
>    diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
>    index 3c3bf37..4f52fa5 100644
>    --- a/src/box/sql/insert.c
>    +++ b/src/box/sql/insert.c
>    @@ -1869,7 +1869,7 @@ xferOptimization(Parse * pParse, /* Parser
>    context */
>       * table (tab1) is initially empty.
>       */
> 
>    - /* The Vdbe we're building*/
>    + /* The Vdbe struct we're building. */
> 
>    You misunderstood me. What I mean is:
>    struct Vibe *v = …;
> 
>      Vdbe *v = sqlite3GetVdbe(pParse);
> 
> References
> 
>    1. mailto:korablev@tarantool.org

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

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]


> 1. Not actual due to 2, but it would be better to use
>   `pOp->p5 &= ~OPFLAG_XFER_OPT` to drop just that flag.
> 2. It is counter-intuitive, IMHO, to change operation flags during that
>   operation. So, said above, vote to move it to OP_OpenWrite.

Well, actually moving it to OP_OpenWrite seems to be bad idea.
Even if code for xFer optimisation is generated, it
still might not be executed. The only opcode ensuring xFer is
under processing - OP_RowData.


[-- Attachment #2: Type: text/html, Size: 3411 bytes --]

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

[Alexander Turenko]

On Sun, Jul 29, 2018 at 02:23:30PM +0300, n.pettik wrote:
> >    1. Not actual due to 2, but it would be better to use
> >      `pOp->p5 &= ~OPFLAG_XFER_OPT` to drop just that flag.
> >    2. It is counter-intuitive, IMHO, to change operation flags during
> >       that operation. So, said above, vote to move it to OP_OpenWrite.
> 
>    Well, actually moving it to OP_OpenWrite seems to be bad idea.
> 
>    Even if code for xFer optimisation is generated, it
> 
>    still might not be executed. The only opcode ensuring xFer is
> 
>    under processing - OP_RowData.

We have separate OpenWrite opcodes in xfer and regular insert code. We
open destination space curson always (to determine whether the space is
empty), but we can set the flag when open source space cursor. But this
will forbid to check source space for emptiness in xfer code or will
require to 'workaround' it using two cursors.

By the way, I observed that the following code is dead:

>         if (emptySrcTest)
>                 sqlite3VdbeJumpHere(v, emptySrcTest);

WBR, Alexander Turenko.

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

[Nikita Tatunov]

[-- Attachment #1: Type: text/plain, Size: 1731 bytes --]

вс, 29 июл. 2018 г. в 18:16, Alexander Turenko <
alexander.turenko@tarantool.org>:

> On Sun, Jul 29, 2018 at 02:23:30PM +0300, n.pettik wrote:
> > >    1. Not actual due to 2, but it would be better to use
> > >      `pOp->p5 &= ~OPFLAG_XFER_OPT` to drop just that flag.
> > >    2. It is counter-intuitive, IMHO, to change operation flags during
> > >       that operation. So, said above, vote to move it to OP_OpenWrite.
> >
> >    Well, actually moving it to OP_OpenWrite seems to be bad idea.
> >
> >    Even if code for xFer optimisation is generated, it
> >
> >    still might not be executed. The only opcode ensuring xFer is
> >
> >    under processing - OP_RowData.
>
> We have separate OpenWrite opcodes in xfer and regular insert code. We
> open destination space curson always (to determine whether the space is
> empty), but we can set the flag when open source space cursor. But this
> will forbid to check source space for emptiness in xfer code or will
> require to 'workaround' it using two cursors.
>
> By the way, I observed that the following code is dead:
>
> >         if (emptySrcTest)
> >                 sqlite3VdbeJumpHere(v, emptySrcTest);
>
> WBR, Alexander Turenko.
>

I'm not sured if changing p5 in OP_OpenWrite is a good idea since
1) Even if p5 isn't used while xfer is processed it's reserved and
there are no free parameters.
2) Who knows, probably we will need p5 for something in future.
3) AFAIK OP_OpenWrite is a legacy opcode and probably gonna be
removed.
4) For me it's more counter-intuitive to put incrementation in
opcode that barely related to xfer optimization idea.
5) Do CPU cycles matter for debugging purpose only global variable?

[-- Attachment #2: Type: text/html, Size: 2259 bytes --]

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

[Alexander Turenko]

On Mon, Jul 30, 2018 at 09:33:55PM +0300, Nikita Tatunov wrote:
>    вс, 29 июл. 2018 г. в 18:16, Alexander Turenko
>    <[1]alexander.turenko@tarantool.org>:
> 
>      On Sun, Jul 29, 2018 at 02:23:30PM +0300, n.pettik wrote:
>      > >    1. Not actual due to 2, but it would be better to use
>      > >      `pOp->p5 &= ~OPFLAG_XFER_OPT` to drop just that flag.
>      > >    2. It is counter-intuitive, IMHO, to change operation flags
>      during
>      > >       that operation. So, said above, vote to move it to
>      OP_OpenWrite.
>      >
>      >    Well, actually moving it to OP_OpenWrite seems to be bad idea.
>      >
>      >    Even if code for xFer optimisation is generated, it
>      >
>      >    still might not be executed. The only opcode ensuring xFer is
>      >
>      >    under processing - OP_RowData.
>      We have separate OpenWrite opcodes in xfer and regular insert code.
>      We
>      open destination space curson always (to determine whether the space
>      is
>      empty), but we can set the flag when open source space cursor. But
>      this
>      will forbid to check source space for emptiness in xfer code or will
>      require to 'workaround' it using two cursors.
>      By the way, I observed that the following code is dead:
>      >         if (emptySrcTest)
>      >                 sqlite3VdbeJumpHere(v, emptySrcTest);
>      WBR, Alexander Turenko.
> 
>    I'm not sured if changing p5 in OP_OpenWrite is a good idea since
>    1) Even if p5 isn't used while xfer is processed it's reserved and
>    there are no free parameters.
>    2) Who knows, probably we will need p5 for something in future.
>    3) AFAIK OP_OpenWrite is a legacy opcode and probably gonna be
>    removed.
>    4) For me it's more counter-intuitive to put incrementation in
>    opcode that barely related to xfer optimization idea.
>    5) Do CPU cycles matter for debugging purpose only global variable?

Ok. So, I think you need to comment in the opcode description that the
flag is cleared after the first opcode execution.

What with the rest of review from the two previous email from me?

WBR, Alexander Turenko.

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

[Nikita Tatunov]

[-- Attachment #1: Type: text/plain, Size: 30826 bytes --]

вс, 29 июл. 2018 г. в 4:12, Alexander Turenko <
alexander.turenko@tarantool.org>:

> Hi!
>
> Please consider my comments and questions below.
>
> WBR, Alexander Turenko.
>
> > +       /*
> > +        * Xfer optimization is unable to correctly insert data
> > +        * in case there's a conflict action other than
> > +        * explicit *_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.
> > +        */
> > +       if (!(onError == ON_CONFLICT_ACTION_ABORT &&
> > +           !is_err_action_default)) {
>
> Do you mean that:
>
> 1. The optimization non-empty table case correctly works only with ABORT
>    conflict action (default or explicit).
> 2. The default conflict action can be overwritten on per-column basis, so
>    'default abort' can really be replace or other conflict action.
>
> If so, your description doesn't give this information.
>
> To more on that, can we check per-column conflict actions instead of check
> whether the conflict action default or explicit? This would enable more
> cases
> with non-empty tables for the optimization. And this would look less
> confusing,
> IMHO.
>

Well, basically, you're right. But the thing is that we're going to remove
column conflict actions, thus, I suppose, it doesn't make sense.
Nikita, Correct me if I'm wrong please.


>
> I have one more question on that. It seems that SQLite has this
> optimization
> working with ROLLBACK conflict action. We cannot doing so, because of some
> problem? Did this problem described / trackerized somewhere? Or something
> changes underhood and makes this impossible? Are we know exact reason or
> just
> observing it does not work?
>

I investigated that a little. OP_IdxInsert was changed so that we can
process
ABORT, FAIL or IGNORE. I took it into account in newer version hence
it is also used in these cases even when the destination table is not empty.


>
> > +#ifdef SQLITE_TEST
> > +/*
> > + * The following global variable is incremented whenever the
> > + * transfer optimization is used.  This is used for testing
> > + * purposes only - to make sure the transfer optimization really
> > + * is happening when it is supposed to.
> > + */
> > +int sql_xfer_count = 0;
> > +#endif
>
> I think it would be good to mention the opcode where the counter is
> incremented. You can follow the style in which other counters are
> described (they are mostly mention opcodes).
>

Added.


>
> > -/* Opcode: RowData P1 P2 * * *
> > +/* Opcode: RowData P1 P2 * * P5
>
> We can increase the counter on per-operation basis instead of per-row by
> adding the flag to OP_OpenWrite. It will save some CPU cycles :)


>
> +#ifdef SQLITE_TEST
> > +       if ((pOp->p5 & OPFLAG_XFER_OPT) != 0) {
> > +               pOp->p5 = 0;
> > +               sql_xfer_count++;
> > +       }
> > +#endif
>
> 1. Not actual due to 2, but it would be better to use
>    `pOp->p5 &= ~OPFLAG_XFER_OPT` to drop just that flag.
> 2. It is counter-intuitive, IMHO, to change operation flags during that
>    operation. So, said above, vote to move it to OP_OpenWrite.
>
>
Made the first.


> > +local bfr, aftr
> > +
>
> What do you plan to do with saved letters? :) Really, such abbreviations
> just makes reading harder with no gains.
>

I made dis :(
(p.s. Changed it)


>
> > +local function do_xfer_test(test_number, return_code)
> > +       test_name = string.format("xfer-optimization-1.%d", test_number)
> > +       test:do_test(
> > +               test_name,
> > +               function()
> > +                       return {aftr - bfr}
> > +               end, {
> > +                       -- <test_name>
> > +                       return_code
> > +                       -- <test_name>
> > +               })
> > +end
>
> That code can be written simpler (consider tap module documentation):
>
> test:is(after - before, exp, test_name)
>
> I suggest to create wrappers like so (I didn't test it):
>
> local function do_xfer_test(test_func, test, test_name, func, exp, opts)
>     local opts = opts or {}
>     local exp_xfer_count = opts.exp_xfer_count
>     local before = box.sql.debug().sql_xfer_count
>     local ok = test_func(test, test_name, func, exp)
>     local after = box.sql.debug().sql_xfer_count
>     if exp_xfer_count ~= nil then
>         ok = ok and test:is(after - before, exp_xfer_count, test_name ..
>             '_xfer_count')
>     end
>     return ok
> end
>
> test.do_execsql_xfer_test = function(test, test_name, func, exp, opts)
>     return do_xfer_test(test.do_execsql_test, test, test_name, func, exp,
> opts)
> end
>
> test.do_catchsql_xfer_test = function(test, test_name, func, exp, opts)
>     return do_xfer_test(test.do_catchsql_test, test, test_name, func, exp,
> opts)
> end
>
> And use it like so:
>
> test:do_catchsql_xfer_test(
>     "xfer-optimization-1.1",
>     [[
>         CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
>         INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
>         CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
>         INSERT INTO t2 SELECT * FROM t1;
>     ]], {
>         -- <xfer-optimization-1.1>
>         0
>         -- <xfer-optimization-1.1>
>     }), {
>         exp_xfer_count = 1
>     }
> )
>
>
Thank you for suggestion. Added that implementation.
(Needed to change it a little bit)


> By the way, you can revive xfer cases in test/sql-tap/with2.test.lua. Or
> drop
> it if your new test includes all related cases from with2.
>
>
They're alive now!


> On Fri, Jul 20, 2018 at 07:58:48PM +0300, Nikita Tatunov wrote:
> >    Ooops. Thank you! fixed it and pushed.
> >
> >    пт, 20 июл. 2018 г. в 19:43, n.pettik <[1]korablev@tarantool.org>:
> >
> >    LGTM.
> >
> >    diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> >    index 3c3bf37..4f52fa5 100644
> >    --- a/src/box/sql/insert.c
> >    +++ b/src/box/sql/insert.c
> >    @@ -1869,7 +1869,7 @@ xferOptimization(Parse * pParse, /* Parser
> >    context */
> >       * table (tab1) is initially empty.
> >       */
> >
> >    - /* The Vdbe we're building*/
> >    + /* The Vdbe struct we're building. */
> >
> >    You misunderstood me. What I mean is:
> >    struct Vibe *v = …;
> >
> >      Vdbe *v = sqlite3GetVdbe(pParse);
> >
> > References
> >
> >    1. mailto:korablev@tarantool.org


Diff:

 diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 3c45920..c3cf94a 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1715,7 +1715,6 @@ xferOptimization(Parse * pParse, /* Parser context */
  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 */
  int regData, regTupleid; /* Registers holding data and tupleid */
  struct session *user_session = current_session();
  bool is_err_action_default = false;
@@ -1881,13 +1880,15 @@ xferOptimization(Parse * pParse, /* Parser context
*/

  /*
  * Xfer optimization is unable to correctly insert data
- * in case there's a conflict action other than
- * explicit *_ABORT. This is the reason we want to only
+ * in case there's a conflict action other than *_ABORT,
+ * *_FAIL or *_IGNORE. 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.
  */
- if (!(onError == ON_CONFLICT_ACTION_ABORT &&
-     !is_err_action_default)) {
+ if (!(onError == ON_CONFLICT_ACTION_ABORT ||
+     onError == ON_CONFLICT_ACTION_FAIL ||
+     onError == ON_CONFLICT_ACTION_IGNORE) ||
+     is_err_action_default) {
  addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iDest, 0);
  VdbeCoverage(v);
  emptyDestTest = sqlite3VdbeAddOp0(v, OP_Goto);
@@ -1905,15 +1906,23 @@ xferOptimization(Parse * pParse, /* Parser context
*/
 #endif

  sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData);
- sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+ switch (onError) {
+ case ON_CONFLICT_ACTION_IGNORE:
+ sqlite3VdbeChangeP5(v, OPFLAG_OE_IGNORE);
+ break;
+ case ON_CONFLICT_ACTION_FAIL:
+ sqlite3VdbeChangeP5(v, OPFLAG_OE_FAIL);
+ break;
+ default:
+ sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+ break;
+ }
  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);
  sqlite3ReleaseTempReg(pParse, regData);
  if (emptyDestTest) {
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index aa7f250..ce01039 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -79,10 +79,10 @@ int sql_search_count = 0;

 #ifdef SQLITE_TEST
 /*
- * The following global variable is incremented whenever the
- * transfer optimization is used.  This is used for testing
- * purposes only - to make sure the transfer optimization really
- * is happening when it is supposed to.
+ * The following global variable is incremented in OP_RowData
+ * whenever the xfer optimization is used. This is used on
+ * testing purposes only - to make sure the transfer optimization
+ * really is happening when it is supposed to.
  */
 int sql_xfer_count = 0;
 #endif
@@ -4008,9 +4008,13 @@ case OP_RowData: {
  BtCursor *pCrsr;
  u32 n;

+/*
+ * Flag P5 is cleared after the first insertion using xfer
+ * optimization.
+ */
 #ifdef SQLITE_TEST
  if ((pOp->p5 & OPFLAG_XFER_OPT) != 0) {
- pOp->p5 = 0;
+ pOp->p5 &= ~OPFLAG_XFER_OPT;
  sql_xfer_count++;
  }
 #endif
diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
index 05d30c6..b99de2b 100755
--- a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
+++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
@@ -2,24 +2,28 @@
 test = require("sqltester")
 test:plan(46)

-local bfr, aftr
-
-local function do_xfer_test(test_number, return_code)
- test_name = string.format("xfer-optimization-1.%d", test_number)
- test:do_test(
- test_name,
- function()
- return {aftr - bfr}
- end, {
- -- <test_name>
- return_code
- -- <test_name>
- })
+local function do_xfer_test(test, test_func, test_name, func, exp, opts)
+ local opts = opts or {}
+ local exp_xfer_count = opts.exp_xfer_count
+ local before = box.sql.debug().sql_xfer_count
+ local ok, result = pcall(test_func, test, test_name, func, exp)
+ local after = box.sql.debug().sql_xfer_count
+ if exp_xfer_count ~= nil then
+ ok = ok and test:is(after - before, exp_xfer_count,
+                     test_name .. '-xfer-count')
+ end
+ return ok
 end

-bfr = box.sql.debug().sql_xfer_count
+test.do_execsql_xfer_test = function(test, test_name, func, exp, opts)
+ return do_xfer_test(test, test.do_execsql_test, test_name, func, exp,
opts)
+end
+
+test.do_catchsql_xfer_test = function(test, test_name, func, exp, opts)
+ return do_xfer_test(test, test.do_catchsql_test, test_name, func, exp,
opts)
+end

-test:do_catchsql_test(
+test:do_catchsql_xfer_test(
  "xfer-optimization-1.1",
  [[
  CREATE TABLE t1(a INTEGER PRIMARY KEY, b INTEGER UNIQUE);
@@ -30,10 +34,10 @@ test:do_catchsql_test(
  -- <xfer-optimization-1.1>
  0
  -- <xfer-optimization-1.1>
+ }, {
+ exp_xfer_count = 1
  })

-aftr = box.sql.debug().sql_xfer_count
-
 test:do_execsql_test(
  "xfer-optimization-1.2",
  [[
@@ -44,12 +48,8 @@ test:do_execsql_test(
  -- <xfer-optimization-1.2>
  })

-do_xfer_test(3, 1)
-
-bfr = box.sql.debug().sql_xfer_count
-
-test:do_catchsql_test(
- "xfer-optimization-1.4",
+test:do_catchsql_xfer_test(
+ "xfer-optimization-1.3",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -60,15 +60,15 @@ test:do_catchsql_test(
  INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3);
  INSERT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.4>
+ -- <xfer-optimization-1.3>
  0
- -- <xfer-optimization-1.4>
+ -- <xfer-optimization-1.3>
+ }, {
+ exp_xfer_count = 1
  })

-aftr = box.sql.debug().sql_xfer_count
-
 test:do_execsql_test(
- "xfer-optimization-1.5",
+ "xfer-optimization-1.4",
  [[
  SELECT * FROM t2;
  ]], {
@@ -77,12 +77,8 @@ test:do_execsql_test(
  -- <xfer-optimization-1.5>
  })

-do_xfer_test(6, 1)
-
-bfr = box.sql.debug().sql_xfer_count
-
-test:do_catchsql_test(
- "xfer-optimization-1.7",
+test:do_catchsql_xfer_test(
+ "xfer-optimization-1.5",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -91,29 +87,25 @@ test:do_catchsql_test(
  CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER);
  INSERT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.7>
+ -- <xfer-optimization-1.5>
  1, "table T2 has 2 columns but 3 values were supplied"
- -- <xfer-optimization-1.7>
+ -- <xfer-optimization-1.5>
+ }, {
+ exp_xfer_count = 0
  })

-aftr = box.sql.debug().sql_xfer_count
-
 test:do_execsql_test(
- "xfer-optimization-1.8",
+ "xfer-optimization-1.6",
  [[
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.8>
+ -- <xfer-optimization-1.6>

- -- <xfer-optimization-1.8>
+ -- <xfer-optimization-1.6>
  })

-do_xfer_test(9, 0)
-
-bfr = box.sql.debug().sql_xfer_count
-
-test:do_catchsql_test(
- "xfer-optimization-1.10",
+test:do_catchsql_xfer_test(
+ "xfer-optimization-1.7",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -122,29 +114,25 @@ test:do_catchsql_test(
  CREATE TABLE t2(a INTEGER PRIMARY KEY, b INTEGER);
  INSERT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.10>
+ -- <xfer-optimization-1.7>
  0
- -- <xfer-optimization-1.10>
+ -- <xfer-optimization-1.7>
+ }, {
+ exp_xfer_count = 1
  })

-aftr = box.sql.debug().sql_xfer_count
-
 test:do_execsql_test(
- "xfer-optimization-1.11",
+ "xfer-optimization-1.8",
  [[
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.11>
+ -- <xfer-optimization-1.8>
  1, 1, 2, 2, 3, 3
- -- <xfer-optimization-1.11>
+ -- <xfer-optimization-1.8>
  })

-do_xfer_test(12, 1);
-
-bfr = box.sql.debug().sql_xfer_count
-
-test:do_catchsql_test(
- "xfer-optimization-1.13",
+test:do_catchsql_xfer_test(
+ "xfer-optimization-1.9",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -153,29 +141,25 @@ test:do_catchsql_test(
  CREATE TABLE t2(b INTEGER, a INTEGER PRIMARY KEY);
  INSERT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.13>
+ -- <xfer-optimization-1.9>
  1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space
'T2'"
- -- <xfer-optimization-1.13>
+ -- <xfer-optimization-1.9>
+ }, {
+ exp_xfer_count = 0
  })

-aftr = box.sql.debug().sql_xfer_count
-
 test:do_execsql_test(
- "xfer-optimization-1.14",
+ "xfer-optimization-1.10",
  [[
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.14>
+ -- <xfer-optimization-1.10>

- -- <xfer-optimization-1.14>
+ -- <xfer-optimization-1.10>
  })

-do_xfer_test(15, 0)
-
-bfr = box.sql.debug().sql_xfer_count
-
-test:do_catchsql_test(
- "xfer-optimization-1.16",
+test:do_catchsql_xfer_test(
+ "xfer-optimization-1.11",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -184,25 +168,23 @@ test:do_catchsql_test(
  CREATE TABLE t2(b INTEGER PRIMARY KEY, a INTEGER);
  INSERT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.16>
+ -- <xfer-optimization-1.11>
  0
- -- <xfer-optimization-1.16>
+ -- <xfer-optimization-1.11>
+ }, {
+ exp_xfer_count = 1
  })

-aftr = box.sql.debug().sql_xfer_count
-
 test:do_execsql_test(
- "xfer-optimization-1.17",
+ "xfer-optimization-1.12",
  [[
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.17>
+ -- <xfer-optimization-1.12>
  1, 1, 2, 2, 3, 2
- -- <xfer-optimization-1.17>
+ -- <xfer-optimization-1.12>
  })

-do_xfer_test(18, 1)
-
 -- 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
@@ -221,10 +203,8 @@ do_xfer_test(18, 1)
 -- 1.0) insert w/o explicit confl. action & w/o index replace action
 ------------------------------------------------------------------------------

-bfr = box.sql.debug().sql_xfer_count
-
-test:do_catchsql_test(
- "xfer-optimization-1.19",
+test:do_catchsql_xfer_test(
+ "xfer-optimization-1.13",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -236,35 +216,31 @@ test:do_catchsql_test(
  INSERT INTO t2 VALUES (4, 4);
  INSERT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.19>
+ -- <xfer-optimization-1.13>
  1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space
'T2'"
- -- <xfer-optimization-1.19>
+ -- <xfer-optimization-1.13>
+ }, {
+ exp_xfer_count = 0
  })

 test:do_execsql_test(
- "xfer-optimization-1.20",
+ "xfer-optimization-1.14",
  [[
  INSERT INTO t2 VALUES (10, 10);
  COMMIT;
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.20>
+ -- <xfer-optimization-1.14>
  2, 2, 3, 4, 4, 4, 10, 10
- -- <xfer-optimization-1.20>
+ -- <xfer-optimization-1.14>
  })

-aftr = box.sql.debug().sql_xfer_count
-
-do_xfer_test(21, 0)
-
 -- 1.1) insert w/o explicit confl. action & w/
 --      index replace action & empty dest_table
 ------------------------------------------------------------------------------

-bfr = box.sql.debug().sql_xfer_count
-
-test:do_catchsql_test(
- "xfer-optimization-1.22",
+test:do_catchsql_xfer_test(
+ "xfer-optimization-1.15",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -276,45 +252,41 @@ test:do_catchsql_test(
  INSERT INTO t3 VALUES (1);
  INSERT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.22>
+ -- <xfer-optimization-1.15>
  0
- -- <xfer-optimization-1.22>
+ -- <xfer-optimization-1.15>
+ }, {
+ exp_xfer_count = 1
  })

 test:do_execsql_test(
- "xfer-optimization-1.23",
+ "xfer-optimization-1.16",
  [[
  INSERT INTO t2 VALUES (10, 10);
  COMMIT;
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.23>
+ -- <xfer-optimization-1.16>
  1, 1, 3, 3, 5, 5, 10, 10
- -- <xfer-optimization-1.23>
+ -- <xfer-optimization-1.16>
  })

-aftr = box.sql.debug().sql_xfer_count
-
 test:do_execsql_test(
- "xfer-optimization-1.24",
+ "xfer-optimization-1.17",
  [[
  SELECT * FROM t3;
  ]], {
- -- <xfer-optimization-1.24>
+ -- <xfer-optimization-1.17>
  1
- -- <xfer-optimization-1.24>
+ -- <xfer-optimization-1.17>
  })

-do_xfer_test(25, 1)
-
 -- 1.2) insert w/o explicit confl. action & w/
 -- index replace action & non-empty dest_table
 ------------------------------------------------------------------------------

-bfr = box.sql.debug().sql_xfer_count
-
-test:do_catchsql_test(
- "xfer-optimization-1.26",
+test:do_catchsql_xfer_test(
+ "xfer-optimization-1.18",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -327,34 +299,30 @@ test:do_catchsql_test(
  INSERT INTO t2 VALUES (4, 4);
  INSERT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.26>
+ -- <xfer-optimization-1.18>
  0
- -- <xfer-optimization-1.26>
+ -- <xfer-optimization-1.18>
+ }, {
+ exp_xfer_count = 0
  })

 test:do_execsql_test(
- "xfer-optimization-1.27",
+ "xfer-optimization-1.19",
  [[
  INSERT INTO t2 VALUES (10, 10);
  COMMIT;
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.27>
+ -- <xfer-optimization-1.19>
  1, 1, 2, 2, 3, 3, 4, 4, 5, 5, 10, 10
- -- <xfer-optimization-1.27>
+ -- <xfer-optimization-1.19>
  })

-aftr = box.sql.debug().sql_xfer_count
-
-do_xfer_test(28, 0)
-
 -- 2) insert with abort
 ------------------------------------------------------------------------------

-bfr = box.sql.debug().sql_xfer_count
-
-test:do_catchsql_test(
- "xfer-optimization-1.29",
+test:do_catchsql_xfer_test(
+ "xfer-optimization-1.20",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -366,34 +334,30 @@ test:do_catchsql_test(
  INSERT INTO t2 VALUES (4, 4);
  INSERT OR ABORT INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.29>
+ -- <xfer-optimization-1.20>
  1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space
'T2'"
- -- <xfer-optimization-1.29>
+ -- <xfer-optimization-1.20>
+ }, {
+ exp_xfer_count = 1
  })

 test:do_execsql_test(
- "xfer-optimization-1.30",
+ "xfer-optimization-1.21",
  [[
  INSERT INTO t2 VALUES (10, 10);
  COMMIT;
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.30>
+ -- <xfer-optimization-1.21>
  2, 2, 3, 4, 4, 4, 10, 10
- -- <xfer-optimization-1.30>
+ -- <xfer-optimization-1.21>
  })

-aftr = box.sql.debug().sql_xfer_count
-
-do_xfer_test(31, 1)
-
 -- 3.0) insert with rollback (into empty table)
 ------------------------------------------------------------------------------

-bfr = box.sql.debug().sql_xfer_count
-
-test:do_catchsql_test(
- "xfer-optimization-1.32",
+test:do_catchsql_xfer_test(
+ "xfer-optimization-1.22",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -403,34 +367,30 @@ test:do_catchsql_test(
  BEGIN;
  INSERT OR ROLLBACK INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.32>
+ -- <xfer-optimization-1.22>
  0
- -- <xfer-optimization-1.32>
+ -- <xfer-optimization-1.22>
+ }, {
+ exp_xfer_count = 1
  })

 test:do_execsql_test(
- "xfer-optimization-1.33",
+ "xfer-optimization-1.23",
  [[
  INSERT INTO t2 VALUES (10, 10);
  COMMIT;
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.33>
+ -- <xfer-optimization-1.23>
  1, 1, 3, 3, 5, 5, 10, 10
- -- <xfer-optimization-1.33>
+ -- <xfer-optimization-1.23>
  })

-aftr = box.sql.debug().sql_xfer_count
-
-do_xfer_test(34, 1)
-
 -- 3.1) insert with rollback (into non-empty table)
 ------------------------------------------------------------------------------

-bfr = box.sql.debug().sql_xfer_count
-
-test:do_catchsql_test(
- "xfer-optimization-1.35",
+test:do_catchsql_xfer_test(
+ "xfer-optimization-1.24",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -442,32 +402,28 @@ test:do_catchsql_test(
  INSERT INTO t2 VALUES (4, 4);
  INSERT OR ROLLBACK INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.35>
+ -- <xfer-optimization-1.24>
  1, "UNIQUE constraint failed: T2.A"
- -- <xfer-optimization-1.35>
+ -- <xfer-optimization-1.24>
+ }, {
+ exp_xfer_count = 0
  })

 test:do_execsql_test(
- "xfer-optimization-1.36",
+ "xfer-optimization-1.25",
  [[
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.36>
+ -- <xfer-optimization-1.25>
  2, 2, 3, 4
- -- <xfer-optimization-1.36>
+ -- <xfer-optimization-1.25>
  })

-aftr = box.sql.debug().sql_xfer_count
-
-do_xfer_test(37, 0)
-
 -- 4) insert with replace
 ------------------------------------------------------------------------------

-bfr = box.sql.debug().sql_xfer_count
-
-test:do_catchsql_test(
- "xfer-optimization-1.38",
+test:do_catchsql_xfer_test(
+ "xfer-optimization-1.26",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -479,34 +435,30 @@ test:do_catchsql_test(
  INSERT INTO t2 VALUES (4, 4);
  INSERT OR REPLACE INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.38>
+ -- <xfer-optimization-1.26>
  0
- -- <xfer-optimization-1.38>
+ -- <xfer-optimization-1.26>
+ }, {
+ exp_xfer_count = 0
  })

 test:do_execsql_test(
- "xfer-optimization-1.39",
+ "xfer-optimization-1.27",
  [[
  INSERT INTO t2 VALUES (10, 10);
  COMMIT;
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.39>
+ -- <xfer-optimization-1.27>
  1, 1, 2, 2, 3, 3, 4, 4, 5, 5, 10, 10
- -- <xfer-optimization-1.39>
+ -- <xfer-optimization-1.27>
  })

-aftr = box.sql.debug().sql_xfer_count
-
-do_xfer_test(40, 0)
-
 -- 5) insert with fail
 ------------------------------------------------------------------------------

-bfr = box.sql.debug().sql_xfer_count
-
-test:do_catchsql_test(
- "xfer-optimization-1.41",
+test:do_catchsql_xfer_test(
+ "xfer-optimization-1.28",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -518,34 +470,30 @@ test:do_catchsql_test(
  INSERT INTO t2 VALUES (4, 4);
  INSERT OR FAIL INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.41>
+ -- <xfer-optimization-1.28>
  1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space
'T2'"
- -- <xfer-optimization-1.41>
+ -- <xfer-optimization-1.28>
+ }, {
+ exp_xfer_count = 1
  })

 test:do_execsql_test(
- "xfer-optimization-1.42",
+ "xfer-optimization-1.29",
  [[
  INSERT INTO t2 VALUES (10, 10);
  COMMIT;
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.42>
+ -- <xfer-optimization-1.29>
  1, 1, 2, 2, 3, 4, 4, 4, 10, 10
- -- <xfer-optimization-1.42>
+ -- <xfer-optimization-1.29>
  })

-aftr = box.sql.debug().sql_xfer_count
-
-do_xfer_test(43, 0)
-
 -- 6) insert with ignore
 ------------------------------------------------------------------------------

-bfr = box.sql.debug().sql_xfer_count
-
-test:do_catchsql_test(
- "xfer-optimization-1.44",
+test:do_catchsql_xfer_test(
+ "xfer-optimization-1.30",
  [[
  DROP TABLE t1;
  DROP TABLE t2;
@@ -557,25 +505,23 @@ test:do_catchsql_test(
  INSERT INTO t2 VALUES (4, 4);
  INSERT OR IGNORE INTO t2 SELECT * FROM t1;
  ]], {
- -- <xfer-optimization-1.44>
+ -- <xfer-optimization-1.30>
  0
- -- <xfer-optimization-1.44>
+ -- <xfer-optimization-1.30>
+ }, {
+ exp_xfer_count = 1
  })

 test:do_execsql_test(
- "xfer-optimization-1.45",
+ "xfer-optimization-1.31",
  [[
  INSERT INTO t2 VALUES (10, 10);
  COMMIT;
  SELECT * FROM t2;
  ]], {
- -- <xfer-optimization-1.45>
+ -- <xfer-optimization-1.31>
  1, 1, 2, 2, 3, 4, 4, 4, 5, 5, 10, 10
- -- <xfer-optimization-1.45>
+ -- <xfer-optimization-1.31>
  })

-aftr = box.sql.debug().sql_xfer_count
-
-do_xfer_test(46, 0)
-
 test:finish_test()
diff --git a/test/sql-tap/with2.test.lua b/test/sql-tap/with2.test.lua
index fbd1f4e..bd0187f 100755
--- a/test/sql-tap/with2.test.lua
+++ b/test/sql-tap/with2.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(44)
+test:plan(59)

 --!./tcltestrunner.lua
 -- 2014 January 11
@@ -382,43 +382,136 @@ genstmt(255), {
 --     -- </4.7>
 -- })

------------------------------------------------------------------------------
--- Check that adding a WITH clause to an INSERT disables the xfer
+-----------------------------------------------------------------
+-- Check that adding a WITH clause to an INSERT disables the xfer
 -- optimization.
---
--- Tarantool: `sqlite3_xferopt_count` is not exported
---   Need to understand if this optimization works at all and if we really
need it.
---   Commented so far.
--- function do_xfer_test(tn, bXfer, sql, res)
---     res = res or ""
---     sqlite3_xferopt_count = 0
---     X(267, "X!cmd", [=[["uplevel",[["list","do_test",["tn"],["\n    set
dres [db eval {",["sql"],"}]\n    list [set ::sqlite3_xferopt_count] [set
dres]\n  "],[["list",["bXfer"],["res"]]]]]]]=])
--- end

--- test:do_execsql_test(
---     5.1,
---     [[
---         DROP TABLE IF EXISTS t1;
---         DROP TABLE IF EXISTS t2;
---         CREATE TABLE t1(a PRIMARY KEY, b);
---         CREATE TABLE t2(a PRIMARY KEY, b);
---     ]])
-
--- do_xfer_test(5.2, 1, " INSERT INTO t1 SELECT * FROM t2 ")
--- do_xfer_test(5.3, 0, " INSERT INTO t1 SELECT a, b FROM t2 ")
--- do_xfer_test(5.4, 0, " INSERT INTO t1 SELECT b, a FROM t2 ")
--- do_xfer_test(5.5, 0, [[
---   WITH x AS (SELECT a, b FROM t2) INSERT INTO t1 SELECT * FROM x
--- ]])
--- do_xfer_test(5.6, 0, [[
---   WITH x AS (SELECT a, b FROM t2) INSERT INTO t1 SELECT * FROM t2
--- ]])
--- do_xfer_test(5.7, 0, [[
---  INSERT INTO t1 WITH x AS ( SELECT * FROM t2 ) SELECT * FROM x
--- ]])
--- do_xfer_test(5.8, 0, [[
---  INSERT INTO t1 WITH x(a,b) AS ( SELECT * FROM t2 ) SELECT * FROM x
--- ]])
+local function do_xfer_test(test, test_func, test_name, func, exp, opts)
+ local opts = opts or {}
+ local exp_xfer_count = opts.exp_xfer_count
+ local before = box.sql.debug().sql_xfer_count
+ local ok, result = pcall(test_func, test, test_name, func, exp)
+ local after = box.sql.debug().sql_xfer_count
+ if exp_xfer_count ~= nil then
+ ok = ok and test:is(after - before, exp_xfer_count,
+                     test_name .. '-xfer-count')
+ end
+ return ok
+end
+
+test.do_execsql_xfer_test = function(test, test_name, func, exp, opts)
+ return do_xfer_test(test, test.do_execsql_test, test_name, func, exp,
opts)
+end
+
+test.do_catchsql_xfer_test = function(test, test_name, func, exp, opts)
+ return do_xfer_test(test, test.do_catchsql_test, test_name, func, exp,
opts)
+end
+
+test:do_execsql_test(
+ 5.1,
+ [[
+ DROP TABLE IF EXISTS t1;
+ DROP TABLE IF EXISTS t2;
+ CREATE TABLE t1(a PRIMARY KEY, b);
+ CREATE TABLE t2(a PRIMARY KEY, b);
+ INSERT INTO t2 VALUES (1, 1), (2, 2);
+ ]], {
+ -- <5.1>
+
+ -- <5.1>
+ })
+
+test:do_execsql_xfer_test(
+ 5.2,
+ [[
+ INSERT INTO t1 SELECT * FROM t2;
+ DELETE FROM t1;
+ ]], {
+ -- <5.2>
+
+ -- <5.2>
+ },  {
+ exp_xfer_count = 1
+ })
+
+test:do_execsql_xfer_test(
+ 5.3,
+ [[
+ INSERT INTO t1 SELECT a, b FROM t2;
+ DELETE FROM t1;
+ ]], {
+ -- <5.3>
+
+ -- <5.3>
+ },  {
+ exp_xfer_count = 0
+ })
+
+test:do_execsql_xfer_test(
+ 5.4,
+ [[
+ INSERT INTO t1 SELECT b, a FROM t2;
+ DELETE FROM t1;
+ ]], {
+ -- <5.4>
+
+ -- <5.4>
+ },  {
+ exp_xfer_count = 0
+ })
+
+test:do_execsql_xfer_test(
+ 5.5,
+ [[
+ WITH x AS (SELECT a, b FROM t2) INSERT INTO t1 SELECT * FROM x;
+ DELETE FROM t1;
+ ]], {
+ -- <5.5>
+
+ -- <5.5>
+ },  {
+ exp_xfer_count = 0
+ })
+
+test:do_execsql_xfer_test(
+ 5.6,
+ [[
+ WITH x AS (SELECT a, b FROM t2) INSERT INTO t1 SELECT * FROM t2;
+ DELETE FROM t1;
+ ]], {
+ -- <5.6>
+
+ -- <5.6>
+ },  {
+ exp_xfer_count = 0
+ })
+
+test:do_execsql_xfer_test(
+ 5.7,
+ [[
+ INSERT INTO t1 WITH x AS (SELECT * FROM t2) SELECT * FROM x;
+ DELETE FROM t1;
+ ]], {
+ -- <5.7>
+
+ -- <5.7>
+ },  {
+ exp_xfer_count = 0
+ })
+
+test:do_execsql_xfer_test(
+ 5.8,
+ [[
+ INSERT INTO t1 WITH x(a,b) AS (SELECT * FROM t2) SELECT * FROM x;
+ DELETE FROM t1;
+ ]], {
+ -- <5.8>
+
+ -- <5.8>
+ },  {
+ exp_xfer_count = 0
+ })
+
 -----------------------------------------------------------------------------
 -- Check that syntax (and other) errors in statements with WITH clauses
 -- attached to them do not cause problems (e.g. memory leaks).

[-- Attachment #2: Type: text/html, Size: 66886 bytes --]

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

[Alexander Turenko]

Hi!

Thanks for updates.

Answered inline.

WBR, Alexander Turenko.

> > > +       /*
> > > +        * Xfer optimization is unable to correctly insert data
> > > +        * in case there's a conflict action other than
> > > +        * explicit *_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.
> > > +        */
> > > +       if (!(onError == ON_CONFLICT_ACTION_ABORT &&
> > > +           !is_err_action_default)) {
> >
> > Do you mean that:
> >
> > 1. The optimization non-empty table case correctly works only with ABORT
> >    conflict action (default or explicit).
> > 2. The default conflict action can be overwritten on per-column basis, so
> >    'default abort' can really be replace or other conflict action.
> >
> > If so, your description doesn't give this information.
> >
> > To more on that, can we check per-column conflict actions instead of check
> > whether the conflict action default or explicit? This would enable more
> > cases
> > with non-empty tables for the optimization. And this would look less
> > confusing,
> > IMHO.
> >
> 
> Well, basically, you're right. But the thing is that we're going to remove
> column conflict actions, thus, I suppose, it doesn't make sense.
> Nikita, Correct me if I'm wrong please.

I recommend to comment such things in the code or commit message (as you
think it is more appropriate). It allows to decrease review iterations
at least :) I don't insist, though.

> > I have one more question on that. It seems that SQLite has this
> > optimization
> > working with ROLLBACK conflict action. We cannot doing so, because of some
> > problem? Did this problem described / trackerized somewhere? Or something
> > changes underhood and makes this impossible? Are we know exact reason or
> > just
> > observing it does not work?
> >
> 
> I investigated that a little. OP_IdxInsert was changed so that we can
> process
> ABORT, FAIL or IGNORE. I took it into account in newer version hence
> it is also used in these cases even when the destination table is not empty.

So I understand it as follows: sqlite3's OP_IdxInsert just copy rows and
cannot handle non-trivial conflict actions, but tarantool's OP_IdxInsert
can handle it. However we have some non-investigated problems with
ROLLBACK and REPLACE as well as per-column conflict actions.

I think that is is worth to investigate it a little deeper and file
issue(s), but this is not the blocker for this patch.

>   sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData);
> - sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
> + switch (onError) {
> + case ON_CONFLICT_ACTION_IGNORE:
> + sqlite3VdbeChangeP5(v, OPFLAG_OE_IGNORE);
> + break;
> + case ON_CONFLICT_ACTION_FAIL:
> + sqlite3VdbeChangeP5(v, OPFLAG_OE_FAIL);
> + break;
> + default:
> + sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
> + break;
> + }

OPFLAG_NCHANGE is independent with other flags I think.

> +local function do_xfer_test(test, test_func, test_name, func, exp, opts)
> + local opts = opts or {}
> + local exp_xfer_count = opts.exp_xfer_count
> + local before = box.sql.debug().sql_xfer_count
> + local ok, result = pcall(test_func, test, test_name, func, exp)
> + local after = box.sql.debug().sql_xfer_count
> + if exp_xfer_count ~= nil then
> + ok = ok and test:is(after - before, exp_xfer_count,
> +                     test_name .. '-xfer-count')
> + end
> + return ok
>  end

I missed that do_execsql_test don't return a result and do_catchsql_test
returns {0} or {1}. So just don't same `ok` and `result` and remove
pcall.

By the way, we have 4 spaces indent for Lua accorsing to Lua Style
Guide.

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

[Nikita Tatunov]

Hello! Thanks for review, Diff is in the end!

вт, 31 июл. 2018 г. в 16:29, Alexander Turenko
<alexander.turenko@tarantool.org>:
>
> Hi!
>
> Thanks for updates.
>
> Answered inline.
>
> WBR, Alexander Turenko.
>
> > > > +       /*
> > > > +        * Xfer optimization is unable to correctly insert data
> > > > +        * in case there's a conflict action other than
> > > > +        * explicit *_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.
> > > > +        */
> > > > +       if (!(onError == ON_CONFLICT_ACTION_ABORT &&
> > > > +           !is_err_action_default)) {
> > >
> > > Do you mean that:
> > >
> > > 1. The optimization non-empty table case correctly works only with ABORT
> > >    conflict action (default or explicit).
> > > 2. The default conflict action can be overwritten on per-column basis, so
> > >    'default abort' can really be replace or other conflict action.
> > >
> > > If so, your description doesn't give this information.
> > >
> > > To more on that, can we check per-column conflict actions instead of check
> > > whether the conflict action default or explicit? This would enable more
> > > cases
> > > with non-empty tables for the optimization. And this would look less
> > > confusing,
> > > IMHO.
> > >
> >
> > Well, basically, you're right. But the thing is that we're going to remove
> > column conflict actions, thus, I suppose, it doesn't make sense.
> > Nikita, Correct me if I'm wrong please.
>
> I recommend to comment such things in the code or commit message (as you
> think it is more appropriate). It allows to decrease review iterations
> at least :) I don't insist, though.
>



> > > I have one more question on that. It seems that SQLite has this
> > > optimization
> > > working with ROLLBACK conflict action. We cannot doing so, because of some
> > > problem? Did this problem described / trackerized somewhere? Or something
> > > changes underhood and makes this impossible? Are we know exact reason or
> > > just
> > > observing it does not work?
> > >
> >
> > I investigated that a little. OP_IdxInsert was changed so that we can
> > process
> > ABORT, FAIL or IGNORE. I took it into account in newer version hence
> > it is also used in these cases even when the destination table is not empty.
>
> So I understand it as follows: sqlite3's OP_IdxInsert just copy rows and
> cannot handle non-trivial conflict actions, but tarantool's OP_IdxInsert
> can handle it. However we have some non-investigated problems with
> ROLLBACK and REPLACE as well as per-column conflict actions.
>
> I think that is is worth to investigate it a little deeper and file
> issue(s), but this is not the blocker for this patch.
>

Yes, I'm on it.

> >   sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData);
> > - sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
> > + switch (onError) {
> > + case ON_CONFLICT_ACTION_IGNORE:
> > + sqlite3VdbeChangeP5(v, OPFLAG_OE_IGNORE);
> > + break;
> > + case ON_CONFLICT_ACTION_FAIL:
> > + sqlite3VdbeChangeP5(v, OPFLAG_OE_FAIL);
> > + break;
> > + default:
> > + sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
> > + break;
> > + }
>
> OPFLAG_NCHANGE is independent with other flags I think.
>

Fixed.

> > +local function do_xfer_test(test, test_func, test_name, func, exp, opts)
> > + local opts = opts or {}
> > + local exp_xfer_count = opts.exp_xfer_count
> > + local before = box.sql.debug().sql_xfer_count
> > + local ok, result = pcall(test_func, test, test_name, func, exp)
> > + local after = box.sql.debug().sql_xfer_count
> > + if exp_xfer_count ~= nil then
> > + ok = ok and test:is(after - before, exp_xfer_count,
> > +                     test_name .. '-xfer-count')
> > + end
> > + return ok
> >  end
>
> I missed that do_execsql_test don't return a result and do_catchsql_test
> returns {0} or {1}. So just don't same `ok` and `result` and remove
> pcall.
>

Fixed.

> By the way, we have 4 spaces indent for Lua accorsing to Lua Style
> Guide.

Fixed.

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index c3cf94a..041601c 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1908,10 +1908,10 @@ xferOptimization(Parse * pParse, /* Parser context */
  sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData);
  switch (onError) {
  case ON_CONFLICT_ACTION_IGNORE:
- sqlite3VdbeChangeP5(v, OPFLAG_OE_IGNORE);
+ sqlite3VdbeChangeP5(v, OPFLAG_OE_IGNORE | OPFLAG_NCHANGE);
  break;
  case ON_CONFLICT_ACTION_FAIL:
- sqlite3VdbeChangeP5(v, OPFLAG_OE_FAIL);
+ sqlite3VdbeChangeP5(v, OPFLAG_OE_FAIL | OPFLAG_NCHANGE);
  break;
  default:
  sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
diff --git a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
index b99de2b..7778fce 100755
--- a/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
+++ b/test/sql-tap/gh-3307-xfer-optimization-issue.test.lua
@@ -6,14 +6,11 @@ local function do_xfer_test(test, test_func,
test_name, func, exp, opts)
     local opts = opts or {}
     local exp_xfer_count = opts.exp_xfer_count
     local before = box.sql.debug().sql_xfer_count
- local ok, result = pcall(test_func, test, test_name, func, exp)
+    test_func(test, test_name, func, exp)
     local after = box.sql.debug().sql_xfer_count
- if exp_xfer_count ~= nil then
- ok = ok and test:is(after - before, exp_xfer_count,
+    return test:is(after - before, exp_xfer_count,
                    test_name .. '-xfer-count')
 end
- return ok
-end

 test.do_execsql_xfer_test = function(test, test_name, func, exp, opts)
     return do_xfer_test(test, test.do_execsql_test, test_name, func, exp, opts)
diff --git a/test/sql-tap/with2.test.lua b/test/sql-tap/with2.test.lua
index bd0187f..a59bac7 100755
--- a/test/sql-tap/with2.test.lua
+++ b/test/sql-tap/with2.test.lua
@@ -390,14 +390,11 @@ local function do_xfer_test(test, test_func,
test_name, func, exp, opts)
     local opts = opts or {}
     local exp_xfer_count = opts.exp_xfer_count
     local before = box.sql.debug().sql_xfer_count
- local ok, result = pcall(test_func, test, test_name, func, exp)
+    test_func(test, test_name, func, exp)
     local after = box.sql.debug().sql_xfer_count
- if exp_xfer_count ~= nil then
- ok = ok and test:is(after - before, exp_xfer_count,
+    return test:is(after - before, exp_xfer_count,
                    test_name .. '-xfer-count')
 end
- return ok
-end

 test.do_execsql_xfer_test = function(test, test_name, func, exp, opts)
     return do_xfer_test(test, test.do_execsql_test, test_name, func, exp, opts)

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

[Alexander Turenko]

Now LGTM.

Kirill, can you look at the patch?

WBR, Alexander Turenko.

On Tue, Jul 31, 2018 at 08:04:32PM +0300, Nikita Tatunov wrote:
> Hello! Thanks for review, Diff is in the end!
> 
> вт, 31 июл. 2018 г. в 16:29, Alexander Turenko
> <alexander.turenko@tarantool.org>:
> >
> > Hi!
> >
> > Thanks for updates.
> >
> > Answered inline.
> >
> > WBR, Alexander Turenko.
> <...>

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

[Kirill Yukhin]

Hello,
On 18 апр 18:32, N.Tatunov wrote:
> 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.
> The bug was fixed so the data should now insert
> correctly.
> 
> Closes #3307
> ---
> 
> Branch: https://github.com/tarantool/tarantool/tree/N_Tatunov/gh-3307-xfer-optimization-issue
> Issue: https://github.com/tarantool/tarantool/issues/3307
I've checked in the patch into 2.0 branch.

--
Regards, Kirill Yukhin