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 D6C0928D5F for ; Wed, 28 Mar 2018 04:58:09 -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 rb5xShRl3CyP for ; Wed, 28 Mar 2018 04:58:09 -0400 (EDT) Received: from smtp15.mail.ru (smtp15.mail.ru [94.100.176.133]) (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 208AB28D4F for ; Wed, 28 Mar 2018 04:58:08 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH] sql: fix non-working REPLACE optimization From: "n.pettik" In-Reply-To: <20180327164227.74619-1-niatshin@tarantool.org> Date: Wed, 28 Mar 2018 11:58:03 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20180327164227.74619-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: Bulat Niatshin Cc: tarantool-patches@freelists.org Hello. Overall comment: functional part of patch seems to be OK (just because tests are passing), but I don=E2=80=99t 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 = wrote: >=20 > 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=E2=80=99t 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. >=20 > Closes #3293 > =E2=80=94 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(-) >=20 > 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 =3D = ON_CONFLICT_ACTION_NONE; > sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, = iDataCur, > iIdxCur, regIns, 0, > ipkColumn >=3D 0, = onError, > - endOfLoop, &isReplace, = 0); > + endOfLoop, &isReplace, = 0, > + = &optimization_availability); > sqlite3FkCheck(pParse, pTab, 0, regIns, 0); >=20 > /* Set the OPFLAG_USESEEKRESULT flag if either (a) there = are no REPLACE > @@ -849,7 +851,8 @@ sqlite3Insert(Parse * pParse, /* Parser = context */ > SQLITE_ForeignKeys) =3D=3D= 0 || > sqlite3FkReferences(pTab) = =3D=3D 0)); > sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx, > - bUseSeek, onError); > + bUseSeek, onError, > + optimization_availability); Rename this variable: generally speaking =E2=80=98availability=E2=80=99 = 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.=20 > } >=20 > /* 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=E2=80=99t 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 =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. > + /** Comments starting from /** should appear only before functions. Inside function start it with /*.=20 > + * 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))) > ) { > + *optimization_opt =3D 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; >=20 > + assert(optimization_availability =3D=3D ON_CONFLICT_ACTION_NONE = || > + optimization_availability =3D=3D = ON_CONFLICT_ACTION_REPLACE || > + optimization_availability =3D=3D = ON_CONFLICT_ACTION_IGNORE); > + > v =3D sqlite3GetVdbe(pParse); > assert(v !=3D 0); > /* This table is not a VIEW */ > @@ -1508,13 +1528,24 @@ sqlite3CompleteInsertion(Parse * pParse, = /* The parser context */ > } > assert(pParse->nested =3D=3D 0); >=20 > - if (onError =3D=3D 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 =3D=3D ON_CONFLICT_ACTION_REPLACE || > + (optimization_availability =3D=3D ON_CONFLICT_ACTION_REPLACE = && > + onError =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 (onError =3D=3D ON_CONFLICT_ACTION_IGNORE || > + (optimization_availability =3D=3D ON_CONFLICT_ACTION_IGNORE = && > + onError =3D=3D ON_CONFLICT_ACTION_DEFAULT)) { > pik_flags |=3D OPFLAG_OE_IGNORE; > } else if (onError =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 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 =3D 0; /* Address of jump instruction = */ > int bReplace =3D 0; /* True if REPLACE conflict = resolution might happen */ > + int optimization_availability =3D = ON_CONFLICT_ACTION_NONE; >=20 > /* 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); >=20 > /* Do FK constraint checks. */ > if (hasFK) { > @@ -616,7 +618,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, > + onError, = optimization_availability); >=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 >=20 >=20