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

[Hollow111 <hollow653@gmail.com>]

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