[patches] Re: [tarantool-patches] [PATCH] sql: fix non-working REPLACE optimization
n.pettik
korablev at tarantool.org
Tue Apr 3 13:20:13 MSK 2018
Hello. See comments below.
> On 30 Mar 2018, at 01:25, Bulat Niatshin <niatshin at tarantool.org> wrote:
>
> In some cases ON CONFLICT REPLACE/IGNORE can be optimized. If
> following conditions are true:
> 1) Space has PRIMARY KEY index only.
> 2) There are no foreign keys to other spaces.
> 3) There are no delete triggers to be fired if conflict happens.
> 4) The errt or action is REPLACE or IGNORE.
errt: typo.
>
> Then uniqueness can be ensured by Tarantool only, without VDBE
> bytecode. This patch contains a small fix for that +
Replace ‘+’ with word. Don’t mention that it is ’small’ fix, let it be just fix.
> related code was refactored a little bit and necessary comments
The same: don’t use ‘little bit’ — it sounds like you left smth
in a shitty state consciously.
> were added.
>
> Closes #3293
> ---
>
> Branch:
> https://github.com/tarantool/tarantool/tree/bn/gh-3293-unique-opt
>
> Issue:
> https://github.com/tarantool/tarantool/issues/3293
>
> src/box/sql/insert.c | 144 ++++++++++++++++++++++++++++++++----------
> src/box/sql/sqliteInt.h | 27 +++++++-
> src/box/sql/update.c | 9 ++-
> test/sql/on-conflict.result | 32 ++++++++++
> test/sql/on-conflict.test.lua | 13 ++++
> 5 files changed, 186 insertions(+), 39 deletions(-)
>
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 0e305d34b..fc9b121b0 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -835,9 +835,13 @@ sqlite3Insert(Parse * pParse, /* Parser context */
> */
> int isReplace; /* Set to true if constraints may cause a replace */
> int bUseSeek; /* True to use OPFLAG_SEEKRESULT */
> +
> + struct on_conflict on_conflict;
> + on_conflict.override_error = onError;
> + on_conflict.optimized_on_conflict = ON_CONFLICT_ACTION_NONE;
> sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, iDataCur,
> iIdxCur, regIns, 0,
> - ipkColumn >= 0, onError,
> + ipkColumn >= 0, &on_conflict,
> endOfLoop, &isReplace, 0);
> sqlite3FkCheck(pParse, pTab, 0, regIns, 0);
>
> @@ -855,7 +859,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */
> SQLITE_ForeignKeys) == 0 ||
> sqlite3FkReferences(pTab) == 0));
> sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx,
> - bUseSeek, onError);
> + bUseSeek, &on_conflict);
> }
>
> /* Update the count of rows that are inserted
> @@ -1056,7 +1060,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */
> int regNewData, /* First register in a range holding values to insert */
> int regOldData, /* Previous content. 0 for INSERTs */
> u8 pkChng, /* Non-zero if the PRIMARY KEY changed */
> - u8 overrideError, /* Override onError to this if not ON_CONFLICT_ACTION_DEFAULT */
> + struct on_conflict *on_conflict,
> 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 */
> @@ -1075,6 +1079,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */
> u8 isUpdate; /* True if this is an UPDATE operation */
> u8 bAffinityDone = 0; /* True if the OP_Affinity operation has been run */
> struct session *user_session = current_session();
> + int override_error = on_conflict->override_error;
>
> isUpdate = regOldData != 0;
> db = pParse->db;
> @@ -1107,8 +1112,8 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */
> continue; /* This column is allowed to be NULL */
>
> onError = table_column_nullable_action(pTab, i);
> - if (overrideError != ON_CONFLICT_ACTION_DEFAULT) {
> - onError = overrideError;
> + if (override_error != ON_CONFLICT_ACTION_DEFAULT) {
> + onError = override_error;
> } else if (onError == ON_CONFLICT_ACTION_DEFAULT) {
> onError = ON_CONFLICT_ACTION_ABORT;
> }
> @@ -1166,9 +1171,12 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */
> SQLITE_IgnoreChecks) == 0) {
> ExprList *pCheck = pTab->pCheck;
> pParse->ckBase = regNewData + 1;
> - onError =
> - overrideError != ON_CONFLICT_ACTION_DEFAULT ? overrideError
> - : ON_CONFLICT_ACTION_ABORT;
> +
> + if (override_error != ON_CONFLICT_ACTION_DEFAULT)
> + onError = override_error;
> + else
> + onError = ON_CONFLICT_ACTION_ABORT;
> +
> for (i = 0; i < pCheck->nExpr; i++) {
> int allOk;
> Expr *pExpr = pCheck->a[i].pExpr;
> @@ -1210,10 +1218,15 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */
> int addrUniqueOk; /* Jump here if the UNIQUE constraint is satisfied */
> bool uniqueByteCodeNeeded = false;
>
> + /*
> + * ABORT and DEFAULT error actions can be handled
> + * by Tarantool only without emitting VDBE
Remove word ‘only’: it confuses little bit (IMHO).
> + * bytecode.
> + */
> if ((pIdx->onError != ON_CONFLICT_ACTION_ABORT &&
> pIdx->onError != ON_CONFLICT_ACTION_DEFAULT) ||
> - (overrideError != ON_CONFLICT_ACTION_ABORT &&
> - overrideError != ON_CONFLICT_ACTION_DEFAULT)) {
> + (override_error != ON_CONFLICT_ACTION_ABORT &&
> + override_error != ON_CONFLICT_ACTION_DEFAULT)) {
> uniqueByteCodeNeeded = true;
> }
>
> @@ -1318,41 +1331,73 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */
> continue;
> }
>
> - /* Find out what action to take in case there is a uniqueness conflict */
> - onError = pIdx->onError;
> - if (onError == ON_CONFLICT_ACTION_NONE) {
> + if (!IsUniqueIndex(pIdx)) {
> sqlite3VdbeResolveLabel(v, addrUniqueOk);
> - continue; /* pIdx is not a UNIQUE index */
> + continue;
> }
> - /* If pIdx is not a UNIQUE or we are doing INSERT OR IGNORE,
> - * INSERT OR FAIL then skip uniqueness checks and let it to be
> - * done by Tarantool.
> +
> + /*
> + * Find out what action to take in case there is
> + * a uniqueness conflict.
> + */
> + onError = pIdx->onError;
> +
Nit-picking: don’t put too many empty lines.
In this particular case, you outline very elementary statement.
> + /*
> + * If we are doing INSERT OR IGNORE,
> + * INSERT OR REPLACE then uniqueness can be
> + * ensured by Tarantool only without bytecode,
> + * because IGNORE/REPLACE error action will be
> + * used for all indexes.
> */
> - if (overrideError == ON_CONFLICT_ACTION_FAIL ||
> - overrideError == ON_CONFLICT_ACTION_IGNORE ||
> - overrideError == ON_CONFLICT_ACTION_ABORT) {
> + if (override_error == ON_CONFLICT_ACTION_FAIL ||
> + override_error == ON_CONFLICT_ACTION_IGNORE ||
> + override_error == ON_CONFLICT_ACTION_ABORT) {
> sqlite3VdbeResolveLabel(v, addrUniqueOk);
> continue; /* pIdx is not a UNIQUE index */
> }
> - if (overrideError != ON_CONFLICT_ACTION_DEFAULT) {
> - onError = overrideError;
> +
> + /*
> + * override_error is an action which is directly
> + * specified by user in queries like
> + * INSERT OR <override_error> and therefore
> + * should have higher priority than indexes
> + * error actions.
> + */
You put almost the same comment to on_error struct.
Change it to more informative or remove.
> + if (override_error != ON_CONFLICT_ACTION_DEFAULT) {
> + onError = override_error;
> } else if (onError == ON_CONFLICT_ACTION_DEFAULT) {
> 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.
> + /*
> + * 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)))
> ) {
> + /*
> + * In that case assign optimized error
> + * action as IGNORE or REPLACE.
> + */
> + on_conflict->optimized_on_conflict = onError;
> +
> sqlite3VdbeResolveLabel(v, addrUniqueOk);
> continue;
> }
> @@ -1425,7 +1470,19 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */
> }
> }
>
> - /* Generate code that executes if the new index entry is not unique */
> + /*
> + * Generate bytecode which executes when entry is
> + * not unique for constraints with following
> + * error actions:
> + * 1) ROLLBACK.
> + * 2) FAIL.
> + * 3) IGNORE.
> + * 4) REPLACE.
> + *
> + * ON CONFLICT ABORT is a default error action
> + * for constraints and therefore can be handled
> + * by Tarantool only.
> + */
> assert(onError == ON_CONFLICT_ACTION_ROLLBACK
> || onError == ON_CONFLICT_ACTION_ABORT
> || onError == ON_CONFLICT_ACTION_FAIL
> @@ -1490,12 +1547,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)
> + struct on_conflict *on_conflict)
> {
> 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;
> + int override_error = on_conflict->override_error;
> + int optimized_error_action = on_conflict->optimized_on_conflict;
If you used enum type, you wouldn’t have to add assertion below.
> +
> + assert(optimized_error_action == ON_CONFLICT_ACTION_NONE ||
> + optimized_error_action == ON_CONFLICT_ACTION_REPLACE ||
> + optimized_error_action == ON_CONFLICT_ACTION_IGNORE);
>
> v = sqlite3GetVdbe(pParse);
> assert(v != 0);
> @@ -1524,15 +1587,26 @@ sqlite3CompleteInsertion(Parse * pParse, /* The parser context */
> }
> assert(pParse->nested == 0);
>
> - if (onError == ON_CONFLICT_ACTION_REPLACE) {
> + /*
> + * override_error represents error action in queries like
> + * INSERT/UPDATE OR REPLACE, INSERT/UPDATE OR IGNORE,
> + * which is strictly specified by user and therefore
> + * should have been used even when optimization is
> + * possible (uniqueness can be checked by Tarantool).
> + */
This comment is almost copy of one from struct declaration.
Remove or rephrase.
> + if (override_error == ON_CONFLICT_ACTION_REPLACE ||
> + (optimized_error_action == ON_CONFLICT_ACTION_REPLACE &&
> + override_error == ON_CONFLICT_ACTION_DEFAULT)) {
> opcode = OP_IdxReplace;
> } else {
> opcode = OP_IdxInsert;
> }
>
> - if (onError == ON_CONFLICT_ACTION_IGNORE) {
> + if (override_error == ON_CONFLICT_ACTION_IGNORE ||
> + (optimized_error_action == ON_CONFLICT_ACTION_IGNORE &&
> + override_error == ON_CONFLICT_ACTION_DEFAULT)) {
> pik_flags |= OPFLAG_OE_IGNORE;
> - } else if (onError == ON_CONFLICT_ACTION_FAIL) {
> + } else if (override_error == ON_CONFLICT_ACTION_FAIL) {
> pik_flags |= OPFLAG_OE_FAIL;
> }
>
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index dc045f97b..5492ca44c 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -666,6 +666,27 @@ enum sql_type {
> SQLITE_NULL = 5,
> };
>
> +/**
> + * Structure for internal usage during INSERT/UPDATE
> + * statements compilation.
> + */
> +struct on_conflict {
> + /**
> + * Represents an error action in queries like
> + * INSERT/UPDATE OR <override_error>, which
> + * overrides all space constraints error actions.
> + */
> + int override_error;
Why do you declare type of this field as int,
when it can take values from on_conflict_action enum?
> + /**
> + * Represents an ON CONFLICT action which can be
> + * optimized and executed without VDBE bytecode, by
> + * Tarantool only. If optimization is not available, then
"Only by Tarantool facilities" would sound better, wouldn’t it?
> + * the value is ON_CONFLICT_ACTION_NONE, otherwise it is
> + * ON_CONFLICT_ACTON_IGNORE or ON_CONFLICT_ACTION_REPLACE.
> + */
> + int optimized_on_conflict;
Your structure is already called ‘on_conflict’,
so don’t repeat this phrase in field’s name.
Also, the same as previous field: better declare it as enum.
> +};
> +
> void *
> sqlite3_user_data(sqlite3_context *);
>
> @@ -3709,8 +3730,10 @@ 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, struct on_conflict *, int,
> + int *, int *);
> +void sqlite3CompleteInsertion(Parse *, Table *, int, int *, int,
> + struct on_conflict *);
> 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..c888aab1a 100644
> --- a/src/box/sql/update.c
> +++ b/src/box/sql/update.c
> @@ -563,11 +563,15 @@ sqlite3Update(Parse * pParse, /* The parser context */
> int addr1 = 0; /* Address of jump instruction */
> int bReplace = 0; /* True if REPLACE conflict resolution might happen */
>
> + struct on_conflict on_conflict;
> + on_conflict.override_error = onError;
> + on_conflict.optimized_on_conflict = ON_CONFLICT_ACTION_NONE;
> +
> /* Do constraint checks. */
> assert(regOldPk > 0);
> sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, iDataCur,
> iIdxCur, regNewPk,
> - regOldPk, chngPk, onError,
> + regOldPk, chngPk, &on_conflict,
> labelContinue, &bReplace,
> aXRef);
>
> @@ -616,7 +620,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,
> + &on_conflict);
>
> /* 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