[tarantool-patches] Re: [PATCH] sql: fix non-working REPLACE optimization
n.pettik
korablev at tarantool.org
Wed Mar 28 11:58:03 MSK 2018
Hello.
Overall comment: functional part of patch seems to be OK
(just because tests are passing), but I don’t like implementation.
Code connected with error actions is very hard to read and understand
owing to mess of actions, constraints order etc. And you make it even worse.
Please, consider some refactoring/renaming/commenting.
> On 27 Mar 2018, at 19:42, Bulat Niatshin <niatshin at tarantool.org> wrote:
>
> SQL PRIMARY KEY constraint with ON CONFLICT REPLACE/IGNORE
> optimization (when error action is REPLACE or IGNORE, table
It would be better firstly to state what optimisation is, then
enumerate conditions which are required, and explain why
this optimisation didn’t work and how you fixed it.
> doesn't have related foreign keys, attached triggers and
> secondary indexes and because of that uniqueness can be checked
> by Tarantool only without bytecode) didn't work. Also ON CONFLICT
> IGNORE uniqueness check can be optimized. This patch contrains a
Typo: contrains.
> small fix for that.
>
> Closes #3293
> —
Put link to the branch and issue here.
> src/box/sql/insert.c | 59 +++++++++++++++++++++++++++++++++----------
> src/box/sql/sqliteInt.h | 4 +--
> src/box/sql/update.c | 7 +++--
> test/sql/on-conflict.result | 32 +++++++++++++++++++++++
> test/sql/on-conflict.test.lua | 13 ++++++++++
> 5 files changed, 97 insertions(+), 18 deletions(-)
>
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 42254ddf4..2c2fc41f2 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -829,10 +829,12 @@ sqlite3Insert(Parse * pParse, /* Parser context */
> */
> int isReplace; /* Set to true if constraints may cause a replace */
> int bUseSeek; /* True to use OPFLAG_SEEKRESULT */
> + int optimization_availability = ON_CONFLICT_ACTION_NONE;
> sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, iDataCur,
> iIdxCur, regIns, 0,
> ipkColumn >= 0, onError,
> - endOfLoop, &isReplace, 0);
> + endOfLoop, &isReplace, 0,
> + &optimization_availability);
> sqlite3FkCheck(pParse, pTab, 0, regIns, 0);
>
> /* Set the OPFLAG_USESEEKRESULT flag if either (a) there are no REPLACE
> @@ -849,7 +851,8 @@ sqlite3Insert(Parse * pParse, /* Parser context */
> SQLITE_ForeignKeys) == 0 ||
> sqlite3FkReferences(pTab) == 0));
> sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx,
> - bUseSeek, onError);
> + bUseSeek, onError,
> + optimization_availability);
Rename this variable: generally speaking ‘availability’ is a boolean
property. But you use it as an error action, i.e. assign to values from enum.
Maybe it is worth to move this flag to parse context, maybe move three flags
(bUseSeek, onError, optimisation_availability) to separate struct or pack
them into one bit-flag-variable.
> }
>
> /* Update the count of rows that are inserted
> @@ -1053,7 +1056,8 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */
> u8 overrideError, /* Override onError to this if not ON_CONFLICT_ACTION_DEFAULT */
> int ignoreDest, /* Jump to this label on an ON_CONFLICT_ACTION_IGNORE resolution */
> int *pbMayReplace, /* OUT: Set to true if constraint may cause a replace */
> - int *aiChng) /* column i is unchanged if aiChng[i]<0 */
> + int *aiChng, /* column i is unchanged if aiChng[i]<0 */
> + int *optimization_opt) /* optimization flag */
Useless comment. Moreover, AFAIK we don’t use such commenting style.
> {
> Vdbe *v; /* VDBE under constrution */
> Index *pIdx; /* Pointer to one of the indices */
> @@ -1324,19 +1328,30 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */
> onError = ON_CONFLICT_ACTION_ABORT;
> }
>
> - /* Collision detection may be omitted if all of the following are true:
> - * (1) The conflict resolution algorithm is REPLACE
> - * (2) There are no secondary indexes on the table
> - * (3) No delete triggers need to be fired if there is a conflict
> - * (4) No FK constraint counters need to be updated if a conflict occurs.
> + /**
Comments starting from /** should appear only before functions.
Inside function start it with /*.
> + * Collision detection may be omitted if all of
> + * the following are true:
> + * (1) The conflict resolution algorithm is
> + * REPLACE or IGNORE.
> + * (2) There are no secondary indexes on the
> + * table.
> + * (3) No delete triggers need to be fired if
> + * there is a conflict.
> + * (4) No FK constraint counters need to be
> + * updated if a conflict occurs.
> */
> if ((ix == 0 && pIdx->pNext == 0) /* Condition 2 */
> - && onError == ON_CONFLICT_ACTION_REPLACE /* Condition 1 */
> - && (0 == (user_session->sql_flags & SQLITE_RecTriggers) /* Condition 3 */
> + /* Condition 1 */
> + && (onError == ON_CONFLICT_ACTION_REPLACE ||
> + onError == ON_CONFLICT_ACTION_IGNORE)
> + /* Condition 3 */
> + && (0 == (user_session->sql_flags & SQLITE_RecTriggers)
> ||0 == sqlite3TriggersExist(pTab, TK_DELETE, 0, 0))
> - && (0 == (user_session->sql_flags & SQLITE_ForeignKeys) || /* Condition 4 */
> + /* Condition 4 */
> + && (0 == (user_session->sql_flags & SQLITE_ForeignKeys) ||
> (0 == pTab->pFKey && 0 == sqlite3FkReferences(pTab)))
> ) {
> + *optimization_opt = onError;
To be honest, this code looks VERY strange,
At least without explanation. Consider renaming/refactoring.
> sqlite3VdbeResolveLabel(v, addrUniqueOk);
> continue;
> }
> @@ -1474,13 +1489,18 @@ sqlite3CompleteInsertion(Parse * pParse, /* The parser context */
> int iIdxCur, /* Primary index cursor */
> int *aRegIdx, /* Register used by each index. 0 for unused indices */
> int useSeekResult, /* True to set the USESEEKRESULT flag on OP_[Idx]Insert */
> - u8 onError)
> + u8 onError,
> + int optimization_availability)
> {
> Vdbe *v; /* Prepared statements under construction */
> Index *pIdx; /* An index being inserted or updated */
> u16 pik_flags; /* flag values passed to the btree insert */
> int opcode;
>
> + assert(optimization_availability == ON_CONFLICT_ACTION_NONE ||
> + optimization_availability == ON_CONFLICT_ACTION_REPLACE ||
> + optimization_availability == ON_CONFLICT_ACTION_IGNORE);
> +
> v = sqlite3GetVdbe(pParse);
> assert(v != 0);
> /* This table is not a VIEW */
> @@ -1508,13 +1528,24 @@ sqlite3CompleteInsertion(Parse * pParse, /* The parser context */
> }
> assert(pParse->nested == 0);
>
> - if (onError == ON_CONFLICT_ACTION_REPLACE) {
> + /**
> + * onError represents override error in queries like
> + * INSERT/UPDATE OR REPLACE, INSERT/UPDATE OR IGNORE.
> + * when error action is strictly specified by user and
> + * therefore should have been used even when optimization
> + * is possible (uniqueness can be checked by Tarantool).
Re-phrase comment: put emphasis on WHY not WHAT
(WHAT can be seen from source code).
> + */
> + if (onError == ON_CONFLICT_ACTION_REPLACE ||
> + (optimization_availability == ON_CONFLICT_ACTION_REPLACE &&
> + onError == ON_CONFLICT_ACTION_DEFAULT)) {
> opcode = OP_IdxReplace;
> } else {
> opcode = OP_IdxInsert;
> }
>
> - if (onError == ON_CONFLICT_ACTION_IGNORE) {
> + if (onError == ON_CONFLICT_ACTION_IGNORE ||
> + (optimization_availability == ON_CONFLICT_ACTION_IGNORE &&
> + onError == ON_CONFLICT_ACTION_DEFAULT)) {
> pik_flags |= OPFLAG_OE_IGNORE;
> } else if (onError == ON_CONFLICT_ACTION_FAIL) {
> pik_flags |= OPFLAG_OE_FAIL;
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index e9c0b3195..cc90be537 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -3702,8 +3702,8 @@ void sqlite3GenerateRowIndexDelete(Parse *, Table *, int, int);
> int sqlite3GenerateIndexKey(Parse *, Index *, int, int, int *, Index *, int);
> void sqlite3ResolvePartIdxLabel(Parse *, int);
> void sqlite3GenerateConstraintChecks(Parse *, Table *, int *, int, int, int,
> - int, u8, u8, int, int *, int *);
> -void sqlite3CompleteInsertion(Parse *, Table *, int, int *, int, u8);
> + int, u8, u8, int, int *, int *, int *);
> +void sqlite3CompleteInsertion(Parse *, Table *, int, int *, int, u8, int);
> int sqlite3OpenTableAndIndices(Parse *, Table *, int, u8, int, u8 *, int *,
> int *, u8, u8);
> void sqlite3BeginWriteOperation(Parse *, int);
> diff --git a/src/box/sql/update.c b/src/box/sql/update.c
> index bf413252d..15e02add8 100644
> --- a/src/box/sql/update.c
> +++ b/src/box/sql/update.c
> @@ -562,6 +562,7 @@ sqlite3Update(Parse * pParse, /* The parser context */
> if (!isView) {
> int addr1 = 0; /* Address of jump instruction */
> int bReplace = 0; /* True if REPLACE conflict resolution might happen */
> + int optimization_availability = ON_CONFLICT_ACTION_NONE;
>
> /* Do constraint checks. */
> assert(regOldPk > 0);
> @@ -569,7 +570,8 @@ sqlite3Update(Parse * pParse, /* The parser context */
> iIdxCur, regNewPk,
> regOldPk, chngPk, onError,
> labelContinue, &bReplace,
> - aXRef);
> + aXRef,
> + &optimization_availability);
>
> /* Do FK constraint checks. */
> if (hasFK) {
> @@ -616,7 +618,8 @@ sqlite3Update(Parse * pParse, /* The parser context */
> }
>
> /* Insert the new index entries and the new record. */
> - sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx, 0, onError);
> + sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx, 0,
> + onError, optimization_availability);
>
> /* Do any ON CASCADE, SET NULL or SET DEFAULT operations required to
> * handle rows (possibly in other tables) that refer via a foreign key
> diff --git a/test/sql/on-conflict.result b/test/sql/on-conflict.result
> index b011a6e6a..5e892eada 100644
> --- a/test/sql/on-conflict.result
> +++ b/test/sql/on-conflict.result
> @@ -50,6 +50,32 @@ box.sql.execute("SELECT * FROM e")
> - [2, 2]
> - [3, 1]
> ...
> +box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY ON CONFLICT REPLACE)")
> +---
> +...
> +box.sql.execute("INSERT INTO t1 VALUES (9)")
> +---
> +...
> +box.sql.execute("INSERT INTO t1 VALUES (9)")
> +---
> +...
> +box.sql.execute("SELECT * FROM t1")
> +---
> +- - [9]
> +...
> +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY ON CONFLICT IGNORE)")
> +---
> +...
> +box.sql.execute("INSERT INTO t2 VALUES (9)")
> +---
> +...
> +box.sql.execute("INSERT INTO t2 VALUES (9)")
> +---
> +...
> +box.sql.execute("SELECT * FROM t2")
> +---
> +- - [9]
> +...
> box.sql.execute('DROP TABLE t')
> ---
> ...
> @@ -62,3 +88,9 @@ box.sql.execute('DROP TABLE p')
> box.sql.execute('DROP TABLE e')
> ---
> ...
> +box.sql.execute('DROP TABLE t1')
> +---
> +...
> +box.sql.execute('DROP TABLE t2')
> +---
> +...
> diff --git a/test/sql/on-conflict.test.lua b/test/sql/on-conflict.test.lua
> index dbf572a8b..1ff199d32 100644
> --- a/test/sql/on-conflict.test.lua
> +++ b/test/sql/on-conflict.test.lua
> @@ -19,7 +19,20 @@ box.sql.execute("SELECT * FROM p")
> box.sql.execute("INSERT INTO e values (1, 1), (2, 2), (3, 1)")
> box.sql.execute("SELECT * FROM e")
>
> +box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY ON CONFLICT REPLACE)")
> +box.sql.execute("INSERT INTO t1 VALUES (9)")
> +box.sql.execute("INSERT INTO t1 VALUES (9)")
> +box.sql.execute("SELECT * FROM t1")
> +
> +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY ON CONFLICT IGNORE)")
> +box.sql.execute("INSERT INTO t2 VALUES (9)")
> +box.sql.execute("INSERT INTO t2 VALUES (9)")
> +
> +box.sql.execute("SELECT * FROM t2")
> +
> box.sql.execute('DROP TABLE t')
> box.sql.execute('DROP TABLE q')
> box.sql.execute('DROP TABLE p')
> box.sql.execute('DROP TABLE e')
> +box.sql.execute('DROP TABLE t1')
> +box.sql.execute('DROP TABLE t2')
> --
> 2.14.1
>
>
More information about the Tarantool-patches
mailing list