[tarantool-patches] Re: [PATCH] sql: xfer optimization issue
n.pettik
korablev at tarantool.org
Thu Jul 19 03:20:48 MSK 2018
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.
More information about the Tarantool-patches
mailing list