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

[Hollow111 <hollow653@gmail.com>]

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