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

  • From: "n.pettik" <korablev@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx
  • Date: Thu, 19 Jul 2018 03:20:48 +0300

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.


Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 07-2018