чт, 19 июл. 2018 г. в 3:20, n.pettik : > 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, { + -- + return_code + -- + }) +end + +bfr = box.sql.debug().sql_xfer_count + test:do_catchsql_test( "xfer-optimization-1.1", [[ @@ -17,6 +37,8 @@ test:do_catchsql_test( -- }) +aftr = box.sql.debug().sql_xfer_count + test:do_execsql_test( "xfer-optimization-1.2", [[ @@ -27,8 +49,12 @@ test:do_execsql_test( -- }) +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; ]], { - -- + -- 0 - -- + -- }) +aftr = box.sql.debug().sql_xfer_count + test:do_execsql_test( - "xfer-optimization-1.4", + "xfer-optimization-1.5", [[ SELECT * FROM t2; ]], { - -- + -- 1, 1, 2, 2, 3, 3 - -- + -- }) +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; ]], { - -- + -- 1, "table T2 has 2 columns but 3 values were supplied" - -- + -- }) +aftr = box.sql.debug().sql_xfer_count + test:do_execsql_test( - "xfer-optimization-1.6", + "xfer-optimization-1.8", [[ SELECT * FROM t2; ]], { - -- + -- - -- + -- }) +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; ]], { - -- + -- 0 - -- + -- }) +aftr = box.sql.debug().sql_xfer_count + test:do_execsql_test( - "xfer-optimization-1.8", + "xfer-optimization-1.11", [[ SELECT * FROM t2; ]], { - -- + -- 1, 1, 2, 2, 3, 3 - -- + -- }) +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; ]], { - -- + -- 1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space 'T2'" - -- + -- }) +aftr = box.sql.debug().sql_xfer_count + test:do_execsql_test( - "xfer-optimization-1.10", + "xfer-optimization-1.14", [[ SELECT * FROM t2; ]], { - -- + -- - -- + -- }) +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; ]], { - -- + -- 0 - -- - }) - -test:do_execsql_test( - "xfer-optimization-1.12", - [[ - SELECT * FROM t2; - ]], { - -- - 1, 1, 2, 2, 3, 2 - -- + -- }) -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; - ]], { - -- - 0 - -- - }) +aftr = box.sql.debug().sql_xfer_count test:do_execsql_test( - "xfer-optimization-1.14", + "xfer-optimization-1.17", [[ SELECT * FROM t2; ]], { - -- - 1, 1, 2, 2, 3, 3, 4, 4, 5, 5 - -- + -- + 1, 1, 2, 2, 3, 2 + -- }) -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; - ]], { - -- - 0 - -- - }) - -test:do_execsql_test( - "xfer-optimization-1.16", - [[ - SELECT * FROM t2; - ]], { - -- - 1, 1, 2, 2, 3, 3, 4, 4, 5, 5 - -- - }) +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, { - -- - return_code - -- - }) -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; ]], { - -- + -- 1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space 'T2'" - -- + -- }) test:do_execsql_test( - "xfer-optimization-1.18", + "xfer-optimization-1.20", [[ INSERT INTO t2 VALUES (10, 10); COMMIT; SELECT * FROM t2; ]], { - -- + -- 2, 2, 3, 4, 4, 4, 10, 10 - -- + -- }) 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; ]], { - -- + -- 0 - -- + -- }) test:do_execsql_test( - "xfer-optimization-1.21", + "xfer-optimization-1.23", [[ INSERT INTO t2 VALUES (10, 10); COMMIT; SELECT * FROM t2; ]], { - -- + -- 1, 1, 3, 3, 5, 5, 10, 10 - -- + -- }) aftr = box.sql.debug().sql_xfer_count test:do_execsql_test( - "xfer-optimization-1.22", + "xfer-optimization-1.24", [[ SELECT * FROM t3; ]], { - -- + -- 1 - -- + -- }) -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; ]], { - -- + -- 0 - -- + -- }) test:do_execsql_test( - "xfer-optimization-1.25", + "xfer-optimization-1.27", [[ INSERT INTO t2 VALUES (10, 10); COMMIT; SELECT * FROM t2; ]], { - -- + -- 1, 1, 2, 2, 3, 3, 4, 4, 5, 5, 10, 10 - -- + -- }) 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; ]], { - -- + -- 1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space 'T2'" - -- + -- }) test:do_execsql_test( - "xfer-optimization-1.28", + "xfer-optimization-1.30", [[ INSERT INTO t2 VALUES (10, 10); COMMIT; SELECT * FROM t2; ]], { - -- + -- 2, 2, 3, 4, 4, 4, 10, 10 - -- + -- }) 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; ]], { - -- + -- 0 - -- + -- }) test:do_execsql_test( - "xfer-optimization-1.31", + "xfer-optimization-1.33", [[ INSERT INTO t2 VALUES (10, 10); COMMIT; SELECT * FROM t2; ]], { - -- + -- 1, 1, 3, 3, 5, 5, 10, 10 - -- + -- }) 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; ]], { - -- + -- 1, "UNIQUE constraint failed: T2.A" - -- + -- }) test:do_execsql_test( - "xfer-optimization-1.34", + "xfer-optimization-1.36", [[ SELECT * FROM t2; ]], { - -- + -- 2, 2, 3, 4 - -- + -- }) 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; ]], { - -- + -- 0 - -- + -- }) test:do_execsql_test( - "xfer-optimization-1.37", + "xfer-optimization-1.39", [[ INSERT INTO t2 VALUES (10, 10); COMMIT; SELECT * FROM t2; ]], { - -- + -- 1, 1, 2, 2, 3, 3, 4, 4, 5, 5, 10, 10 - -- + -- }) 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; ]], { - -- + -- 1, "Duplicate key exists in unique index 'sqlite_autoindex_T2_1' in space 'T2'" - -- + -- }) test:do_execsql_test( - "xfer-optimization-1.40", + "xfer-optimization-1.42", [[ INSERT INTO t2 VALUES (10, 10); COMMIT; SELECT * FROM t2; ]], { - -- + -- 1, 1, 2, 2, 3, 4, 4, 4, 10, 10 - -- + -- }) 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; ]], { - -- + -- 0 - -- + -- }) test:do_execsql_test( - "xfer-optimization-1.43", + "xfer-optimization-1.45", [[ INSERT INTO t2 VALUES (10, 10); COMMIT; SELECT * FROM t2; ]], { - -- + -- 1, 1, 2, 2, 3, 4, 4, 4, 5, 5, 10, 10 - -- + -- }) aftr = box.sql.debug().sql_xfer_count -do_xfer_test(44, 0) +do_xfer_test(46, 0) test:finish_test()