From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id D59B92C434 for ; Tue, 3 Apr 2018 07:02:38 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id guZbkYLOA1uV for ; Tue, 3 Apr 2018 07:02:38 -0400 (EDT) Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 1B0D32C42D for ; Tue, 3 Apr 2018 07:02:36 -0400 (EDT) From: "n.pettik" Message-Id: <940888C6-046C-4DC5-9829-4539ADFC4DFE@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_60B6604A-598F-444B-A901-C1ABE191E09B" Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [patches] [PATCH] sql: fix non-working REPLACE optimization Date: Tue, 3 Apr 2018 14:02:31 +0300 In-Reply-To: References: <20180329222509.69673-1-niatshin@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Bulat Niatshin --Apple-Mail=_60B6604A-598F-444B-A901-C1ABE191E09B Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Accidentally I have sent patch to outdate mailing list. Resending it again to the new one. Reply with fixes to this mail. > On 3 Apr 2018, at 13:20, n.pettik wrote: >=20 > Hello. See comments below. >=20 >> On 30 Mar 2018, at 01:25, Bulat Niatshin > wrote: >>=20 >> 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. >=20 > errt: typo. >=20 >>=20 >> Then uniqueness can be ensured by Tarantool only, without VDBE >> bytecode. This patch contains a small fix for that + >=20 > Replace =E2=80=98+=E2=80=99 with word. Don=E2=80=99t mention that it = is =E2=80=99small=E2=80=99 fix, let it be just fix. >=20 >> related code was refactored a little bit and necessary comments >=20 > The same: don=E2=80=99t use =E2=80=98little bit=E2=80=99 =E2=80=94 it = sounds like you left smth > in a shitty state consciously. >=20 >> were added. >>=20 >> Closes #3293 >> --- >>=20 >> Branch: >> https://github.com/tarantool/tarantool/tree/bn/gh-3293-unique-opt >>=20 >> Issue: >> https://github.com/tarantool/tarantool/issues/3293 >>=20 >> 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(-) >>=20 >> 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 =3D onError; >> + on_conflict.optimized_on_conflict =3D = ON_CONFLICT_ACTION_NONE; >> sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, = iDataCur, >> iIdxCur, regIns, 0, >> - ipkColumn >=3D 0, = onError, >> + ipkColumn >=3D 0, = &on_conflict, >> endOfLoop, &isReplace, = 0); >> sqlite3FkCheck(pParse, pTab, 0, regIns, 0); >>=20 >> @@ -855,7 +859,7 @@ sqlite3Insert(Parse * pParse, /* Parser = context */ >> SQLITE_ForeignKeys) =3D=3D= 0 || >> sqlite3FkReferences(pTab) = =3D=3D 0)); >> sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx, >> - bUseSeek, onError); >> + bUseSeek, &on_conflict); >> } >>=20 >> /* 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 =3D 0; /* True if the OP_Affinity operation has = been run */ >> struct session *user_session =3D current_session(); >> + int override_error =3D on_conflict->override_error; >>=20 >> isUpdate =3D regOldData !=3D 0; >> db =3D pParse->db; >> @@ -1107,8 +1112,8 @@ sqlite3GenerateConstraintChecks(Parse * pParse, = /* The parser context */ >> continue; /* This column is allowed to be = NULL */ >>=20 >> onError =3D table_column_nullable_action(pTab, i); >> - if (overrideError !=3D ON_CONFLICT_ACTION_DEFAULT) { >> - onError =3D overrideError; >> + if (override_error !=3D ON_CONFLICT_ACTION_DEFAULT) { >> + onError =3D override_error; >> } else if (onError =3D=3D ON_CONFLICT_ACTION_DEFAULT) { >> onError =3D ON_CONFLICT_ACTION_ABORT; >> } >> @@ -1166,9 +1171,12 @@ sqlite3GenerateConstraintChecks(Parse * = pParse, /* The parser context */ >> SQLITE_IgnoreChecks) =3D=3D 0) { >> ExprList *pCheck =3D pTab->pCheck; >> pParse->ckBase =3D regNewData + 1; >> - onError =3D >> - overrideError !=3D ON_CONFLICT_ACTION_DEFAULT ? = overrideError >> - : ON_CONFLICT_ACTION_ABORT; >> + >> + if (override_error !=3D ON_CONFLICT_ACTION_DEFAULT) >> + onError =3D override_error; >> + else >> + onError =3D ON_CONFLICT_ACTION_ABORT; >> + >> for (i =3D 0; i < pCheck->nExpr; i++) { >> int allOk; >> Expr *pExpr =3D 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 =3D false; >>=20 >> + /* >> + * ABORT and DEFAULT error actions can be handled >> + * by Tarantool only without emitting VDBE >=20 > Remove word =E2=80=98only=E2=80=99: it confuses little bit (IMHO). >=20 >> + * bytecode. >> + */ >> if ((pIdx->onError !=3D ON_CONFLICT_ACTION_ABORT && >> pIdx->onError !=3D ON_CONFLICT_ACTION_DEFAULT) || >> - (overrideError !=3D ON_CONFLICT_ACTION_ABORT && >> - overrideError !=3D ON_CONFLICT_ACTION_DEFAULT)) { >> + (override_error !=3D ON_CONFLICT_ACTION_ABORT && >> + override_error !=3D ON_CONFLICT_ACTION_DEFAULT)) { >> uniqueByteCodeNeeded =3D true; >> } >>=20 >> @@ -1318,41 +1331,73 @@ sqlite3GenerateConstraintChecks(Parse * = pParse, /* The parser context */ >> continue; >> } >>=20 >> - /* Find out what action to take in case there is a = uniqueness conflict */ >> - onError =3D pIdx->onError; >> - if (onError =3D=3D 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 =3D pIdx->onError; >> + >=20 > Nit-picking: don=E2=80=99t put too many empty lines. > In this particular case, you outline very elementary statement. >=20 >> + /* >> + * 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 =3D=3D ON_CONFLICT_ACTION_FAIL || >> - overrideError =3D=3D ON_CONFLICT_ACTION_IGNORE || >> - overrideError =3D=3D ON_CONFLICT_ACTION_ABORT) { >> + if (override_error =3D=3D ON_CONFLICT_ACTION_FAIL || >> + override_error =3D=3D ON_CONFLICT_ACTION_IGNORE || >> + override_error =3D=3D ON_CONFLICT_ACTION_ABORT) { >> sqlite3VdbeResolveLabel(v, addrUniqueOk); >> continue; /* pIdx is not a UNIQUE index */ >> } >> - if (overrideError !=3D ON_CONFLICT_ACTION_DEFAULT) { >> - onError =3D overrideError; >> + >> + /* >> + * override_error is an action which is directly >> + * specified by user in queries like >> + * INSERT OR and therefore >> + * should have higher priority than indexes >> + * error actions. >> + */ >=20 > You put almost the same comment to on_error struct. > Change it to more informative or remove. >=20 >> + if (override_error !=3D ON_CONFLICT_ACTION_DEFAULT) { >> + onError =3D override_error; >> } else if (onError =3D=3D ON_CONFLICT_ACTION_DEFAULT) { >> onError =3D ON_CONFLICT_ACTION_ABORT; >> } >>=20 >> - /* 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 =3D=3D 0 && pIdx->pNext =3D=3D 0) /* = Condition 2 */ >> - && onError =3D=3D ON_CONFLICT_ACTION_REPLACE = /* Condition 1 */ >> - && (0 =3D=3D (user_session->sql_flags & = SQLITE_RecTriggers) /* Condition 3 */ >> + /* Condition 1 */ >> + && (onError =3D=3D ON_CONFLICT_ACTION_REPLACE || >> + onError =3D=3D ON_CONFLICT_ACTION_IGNORE) >> + /* Condition 3 */ >> + && (0 =3D=3D (user_session->sql_flags & = SQLITE_RecTriggers) >> ||0 =3D=3D sqlite3TriggersExist(pTab, TK_DELETE, = 0, 0)) >> - && (0 =3D=3D (user_session->sql_flags & = SQLITE_ForeignKeys) || /* Condition 4 */ >> + /* Condition 4 */ >> + && (0 =3D=3D (user_session->sql_flags & = SQLITE_ForeignKeys) || >> (0 =3D=3D pTab->pFKey && 0 =3D=3D = sqlite3FkReferences(pTab))) >> ) { >> + /* >> + * In that case assign optimized error >> + * action as IGNORE or REPLACE. >> + */ >> + on_conflict->optimized_on_conflict =3D onError; >> + >> sqlite3VdbeResolveLabel(v, addrUniqueOk); >> continue; >> } >> @@ -1425,7 +1470,19 @@ sqlite3GenerateConstraintChecks(Parse * = pParse, /* The parser context */ >> } >> } >>=20 >> - /* 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 =3D=3D ON_CONFLICT_ACTION_ROLLBACK >> || onError =3D=3D ON_CONFLICT_ACTION_ABORT >> || onError =3D=3D 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 =3D on_conflict->override_error; >> + int optimized_error_action =3D = on_conflict->optimized_on_conflict; >=20 > If you used enum type, you wouldn=E2=80=99t have to add assertion = below. >=20 >> + >> + assert(optimized_error_action =3D=3D ON_CONFLICT_ACTION_NONE || >> + optimized_error_action =3D=3D ON_CONFLICT_ACTION_REPLACE = || >> + optimized_error_action =3D=3D ON_CONFLICT_ACTION_IGNORE); >>=20 >> v =3D sqlite3GetVdbe(pParse); >> assert(v !=3D 0); >> @@ -1524,15 +1587,26 @@ sqlite3CompleteInsertion(Parse * pParse, = /* The parser context */ >> } >> assert(pParse->nested =3D=3D 0); >>=20 >> - if (onError =3D=3D 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). >> + */ >=20 > This comment is almost copy of one from struct declaration.=20 > Remove or rephrase. >=20 >> + if (override_error =3D=3D ON_CONFLICT_ACTION_REPLACE || >> + (optimized_error_action =3D=3D ON_CONFLICT_ACTION_REPLACE && >> + override_error =3D=3D ON_CONFLICT_ACTION_DEFAULT)) { >> opcode =3D OP_IdxReplace; >> } else { >> opcode =3D OP_IdxInsert; >> } >>=20 >> - if (onError =3D=3D ON_CONFLICT_ACTION_IGNORE) { >> + if (override_error =3D=3D ON_CONFLICT_ACTION_IGNORE || >> + (optimized_error_action =3D=3D ON_CONFLICT_ACTION_IGNORE && >> + override_error =3D=3D ON_CONFLICT_ACTION_DEFAULT)) { >> pik_flags |=3D OPFLAG_OE_IGNORE; >> - } else if (onError =3D=3D ON_CONFLICT_ACTION_FAIL) { >> + } else if (override_error =3D=3D ON_CONFLICT_ACTION_FAIL) { >> pik_flags |=3D OPFLAG_OE_FAIL; >> } >>=20 >> 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 =3D 5, >> }; >>=20 >> +/** >> + * Structure for internal usage during INSERT/UPDATE >> + * statements compilation. >> + */ >> +struct on_conflict { >> + /** >> + * Represents an error action in queries like >> + * INSERT/UPDATE OR , which >> + * overrides all space constraints error actions. >> + */ >> + int override_error; >=20 > Why do you declare type of this field as int, > when it can take values from on_conflict_action enum? >=20 >> + /** >> + * Represents an ON CONFLICT action which can be >> + * optimized and executed without VDBE bytecode, by >> + * Tarantool only. If optimization is not available, then >=20 > "Only by Tarantool facilities" would sound better, wouldn=E2=80=99t = it? >=20 >> + * the value is ON_CONFLICT_ACTION_NONE, otherwise it is >> + * ON_CONFLICT_ACTON_IGNORE or ON_CONFLICT_ACTION_REPLACE. >> + */ >> + int optimized_on_conflict; >=20 > Your structure is already called =E2=80=98on_conflict=E2=80=99, > so don=E2=80=99t repeat this phrase in field=E2=80=99s name. > Also, the same as previous field: better declare it as enum. >=20 >> +}; >> + >> void * >> sqlite3_user_data(sqlite3_context *); >>=20 >> @@ -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 =3D 0; /* Address of jump instruction = */ >> int bReplace =3D 0; /* True if REPLACE conflict = resolution might happen */ >>=20 >> + struct on_conflict on_conflict; >> + on_conflict.override_error =3D onError; >> + on_conflict.optimized_on_conflict =3D = 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); >>=20 >> @@ -616,7 +620,8 @@ sqlite3Update(Parse * pParse, /* The = parser context */ >> } >>=20 >> /* Insert the new index entries and the new record. */ >> - sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx, = 0, onError); >> + sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx, = 0, >> + &on_conflict); >>=20 >> /* 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") >>=20 >> +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') >> --=20 >> 2.14.1 --Apple-Mail=_60B6604A-598F-444B-A901-C1ABE191E09B Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
Accidentally I have sent patch to outdate = mailing list.
Resending it again to the new = one.
Reply with fixes to this mail.

On = 3 Apr 2018, at 13:20, n.pettik <korablev@tarantool.org> wrote:

Hello. See comments = below.

On 30 Mar 2018, at 01:25, = Bulat Niatshin <niatshin@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 =E2=80=98+=E2=80=99 with word. = Don=E2=80=99t mention that it is =E2=80=99small=E2=80=99 fix, let it be = just fix.

related code was refactored = a little bit and necessary comments

The same: don=E2=80=99t use =E2=80=98little = bit=E2=80=99 =E2=80=94 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-o= pt

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 =3D onError;
+ = on_conflict.optimized_on_conflict =3D ON_CONFLICT_ACTION_NONE;
= = sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, = iDataCur,
iIdxCur, regIns, 0,
- = = = = = = ipkColumn >=3D 0, onError,
+ ipkColumn = >=3D 0, &on_conflict,
endOfLoop, &isReplace, 0);
= = sqlite3FkCheck(pParse, pTab, 0, regIns, 0);

@@ -855,7 +859,7 @@ sqlite3Insert(Parse * pParse, /* Parser = context */
SQLITE_ForeignKeys) =3D=3D 0 = ||
      = ; sqlite3FkReferences(pTab) =3D=3D 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 =3D 0; /* True if the OP_Affinity operation has been run */
= struct session *user_session =3D current_session();
+ = int override_error =3D on_conflict->override_error;

isUpdate =3D regOldData !=3D = 0;
db =3D pParse->db;
@@ -1107,8 +1112,8 @@ = sqlite3GenerateConstraintChecks(Parse * pParse, /* The = parser context */
continue; /* This = column is allowed to be NULL */

onError =3D= table_column_nullable_action(pTab, i);
- if = (overrideError !=3D ON_CONFLICT_ACTION_DEFAULT) {
- onError =3D= overrideError;
+ if (override_error !=3D = ON_CONFLICT_ACTION_DEFAULT) {
+ onError =3D= override_error;
} else if (onError =3D=3D = ON_CONFLICT_ACTION_DEFAULT) {
onError =3D = ON_CONFLICT_ACTION_ABORT;
}
@@ -1166,9 = +1171,12 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The = parser context */
     SQLIT= E_IgnoreChecks) =3D=3D 0) {
ExprList *pCheck =3D = pTab->pCheck;
pParse->ckBase =3D regNewData = + 1;
- onError =3D
-     overrideErr= or !=3D ON_CONFLICT_ACTION_DEFAULT ? overrideError
- : = ON_CONFLICT_ACTION_ABORT;
+
+ if = (override_error !=3D ON_CONFLICT_ACTION_DEFAULT)
+ onError =3D= override_error;
+ else
+ onError =3D= ON_CONFLICT_ACTION_ABORT;
+
for (i =3D = 0; i < pCheck->nExpr; i++) {
int = allOk;
Expr *pExpr =3D 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 =3D false;

+ = = /*
+  * ABORT and DEFAULT error = actions can be handled
+  * by Tarantool only without = emitting VDBE

Remove word =E2=80=98only=E2=80=99= : it confuses little bit (IMHO).

+  * bytecode.
+ = =  */
= = if ((pIdx->onError !=3D ON_CONFLICT_ACTION_ABORT &&
= =      pIdx-= >onError !=3D ON_CONFLICT_ACTION_DEFAULT) ||
-     (overrideEr= ror !=3D ON_CONFLICT_ACTION_ABORT &&
-      overr= ideError !=3D ON_CONFLICT_ACTION_DEFAULT)) {
+     (override_e= rror !=3D ON_CONFLICT_ACTION_ABORT &&
+      overr= ide_error !=3D ON_CONFLICT_ACTION_DEFAULT)) {
= uniqueByteCodeNeeded =3D 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 =3D= pIdx->onError;
- if (onError =3D=3D = 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 =3D pIdx->onError;
+

Nit-picking: don=E2=80=99t 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 =3D=3D ON_CONFLICT_ACTION_FAIL ||
- = =     overrideErr= or =3D=3D ON_CONFLICT_ACTION_IGNORE ||
-     overrideErr= or =3D=3D ON_CONFLICT_ACTION_ABORT) {
+ if = (override_error =3D=3D ON_CONFLICT_ACTION_FAIL ||
+     override_er= ror =3D=3D ON_CONFLICT_ACTION_IGNORE ||
+     override_er= ror =3D=3D ON_CONFLICT_ACTION_ABORT) {
= sqlite3VdbeResolveLabel(v, addrUniqueOk);
= continue; /* pIdx is not a UNIQUE index */
}
- = = if (overrideError !=3D ON_CONFLICT_ACTION_DEFAULT) {
- = = = onError =3D 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 !=3D ON_CONFLICT_ACTION_DEFAULT) {
+ onError =3D= override_error;
} else if (onError =3D=3D = ON_CONFLICT_ACTION_DEFAULT) {
onError =3D = 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 =3D=3D 0 && pIdx->pNext =3D=3D 0) /* = Condition 2 */
-     && = onError =3D=3D ON_CONFLICT_ACTION_REPLACE /* Condition 1 */
- = =     && = (0 =3D=3D (user_session->sql_flags & SQLITE_RecTriggers) /* = Condition 3 */
+     /* = Condition 1 */
+     && = (onError =3D=3D ON_CONFLICT_ACTION_REPLACE ||
+ onError = =3D=3D ON_CONFLICT_ACTION_IGNORE)
+     /* = Condition 3 */
+     && = (0 =3D=3D (user_session->sql_flags & SQLITE_RecTriggers)
= = = ||0 =3D=3D sqlite3TriggersExist(pTab, TK_DELETE, 0, 0))
- = =     && = (0 =3D=3D (user_session->sql_flags & SQLITE_ForeignKeys) || /* = Condition 4 */
+     /* = Condition 4 */
+     && = (0 =3D=3D (user_session->sql_flags & SQLITE_ForeignKeys) ||
= = = (0 =3D=3D pTab->pFKey && 0 =3D=3D = sqlite3FkReferences(pTab)))
    ) {
+ = = = /*
+  * In that case assign = optimized error
+  * action as IGNORE or = REPLACE.
+  */
+ = on_conflict->optimized_on_conflict =3D 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 =3D=3D ON_CONFLICT_ACTION_ROLLBACK
= =       = ; || onError =3D=3D ON_CONFLICT_ACTION_ABORT
      = ; || onError =3D=3D 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 =3D = on_conflict->override_error;
+ int = optimized_error_action =3D on_conflict->optimized_on_conflict;

If you used enum type, you wouldn=E2=80=99t= have to add assertion below.

+
+ = assert(optimized_error_action =3D=3D ON_CONFLICT_ACTION_NONE = ||
+       = ; optimized_error_action =3D=3D ON_CONFLICT_ACTION_REPLACE ||
+ =       = ; optimized_error_action =3D=3D ON_CONFLICT_ACTION_IGNORE);

v =3D sqlite3GetVdbe(pParse);
= assert(v !=3D 0);
@@ -1524,15 +1587,26 @@ = sqlite3CompleteInsertion(Parse * pParse, /* The parser context */
= }
assert(pParse->nested =3D=3D = 0);

- if (onError =3D=3D = 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 =3D=3D ON_CONFLICT_ACTION_REPLACE ||
+     (optimized_= error_action =3D=3D ON_CONFLICT_ACTION_REPLACE &&
+ =      overr= ide_error =3D=3D ON_CONFLICT_ACTION_DEFAULT)) {
opcode =3D = OP_IdxReplace;
} else {
opcode =3D = OP_IdxInsert;
}

- = if (onError =3D=3D ON_CONFLICT_ACTION_IGNORE) {
+ = if (override_error =3D=3D ON_CONFLICT_ACTION_IGNORE ||
+ =     (optimized_= error_action =3D=3D ON_CONFLICT_ACTION_IGNORE &&
+ =      overr= ide_error =3D=3D ON_CONFLICT_ACTION_DEFAULT)) {
pik_flags = |=3D OPFLAG_OE_IGNORE;
- } else if (onError =3D=3D = ON_CONFLICT_ACTION_FAIL) {
+ } else if (override_error =3D=3D = ON_CONFLICT_ACTION_FAIL) {
pik_flags |=3D 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 =3D 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=E2=80=99t 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 = =E2=80=98on_conflict=E2=80=99,
so don=E2=80=99t repeat this = phrase in field=E2=80=99s 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 =3D 0; /* Address of jump instruction = */
int bReplace =3D 0; /* True if REPLACE conflict = resolution might happen */

+ struct = on_conflict on_conflict;
+ on_conflict.override_error =3D = onError;
+ on_conflict.optimized_on_conflict = =3D 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

= --Apple-Mail=_60B6604A-598F-444B-A901-C1ABE191E09B--