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 2262F266A0 for ; Wed, 4 Apr 2018 07:51:06 -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 IQY4Ks8d9tRP for ; Wed, 4 Apr 2018 07:51:06 -0400 (EDT) Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (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 9F4D72655B for ; Wed, 4 Apr 2018 07:51:05 -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 1/4] sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace] From: "n.pettik" In-Reply-To: <990a2f61-37b2-e986-8986-9ef2d90781b0@tarantool.org> Date: Wed, 4 Apr 2018 14:51:00 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <2408347A-7B2A-4FD6-9699-CAC2ED2F0544@tarantool.org> References: <2b18eeeaf3c68c8c33de3dd7c8e59eac10b3bd8c.1522769319.git.v.shpilevoy@tarantool.org> <990a2f61-37b2-e986-8986-9ef2d90781b0@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: Vladislav Shpilevoy Cc: tarantool-patches@freelists.org You may not pay attention to =E2=80=99nitpickings=E2=80=99, but I should make visibility of reviewing this patch. > On 4 Apr 2018, at 13:26, Vladislav Shpilevoy = wrote: >=20 > Hello. See the diff at the end of the letter, and my responses to some = of > your comments. >>> done in a primary index only. It is enough to pass a register of >>> a primary index. >> Nitpicking: not 'a register of a primary index=E2=80=99, >> but number of cursor opened on this space/index. > Here you are wrong - it is a register, where tuple data is stored to = insert. Cursor id > argument is not changed. I fixed it in a new commit. Ok, I have misunderstood you. Now it is clear that you mentioned another operand. >>=20 >>> + bool no_foreign_keys =3D >>> + sqlite3FkReferences(pTab) =3D=3D NULL || >>> + (user_session->sql_flags & SQLITE_ForeignKeys) = =3D=3D 0; >> AFAIK SQLITE_ForeignKeys flag is always set to be true >> (since we always use FK). Thus, you can remove this condition check. > Yes, Foreign Keys source code now is always built, because we have = removed OMIT_FOREIGN_KEYS, > but SQLITE_ForeignKeys is not a #define - it is a flag, that can be = set by a user on runtime in its > session, and allows to skip foreign keys checking. The removal of this = flags must be discussed, > because I think it can be useful. >=20 > In any case, talking about the patch, I found a way to remove this = code completely. See diff below. Ok, looks pretty good. >=20 >=20 > diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c > index c5dcedd3e..4eae896ab 100644 > --- a/src/box/sql/analyze.c > +++ b/src/box/sql/analyze.c > @@ -1050,7 +1050,6 @@ analyzeOneTable(Parse * pParse, /* Parser = context */ > sqlite3VdbeAddOp4(v, OP_MakeRecord, regTabname, 3, = regTemp, > "BBB", 0); > sqlite3VdbeAddOp2(v, OP_IdxInsert, iStatCur, regTemp); > - sqlite3VdbeChangeP5(v, OPFLAG_APPEND); > /* Add the entries to the stat4 table. */ > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 5e3ed0f39..5c3cac881 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -2659,7 +2659,6 @@ sqlite3RefillIndex(Parse * pParse, Index * = pIndex, int memRootPage) > sqlite3VdbeAddOp3(v, OP_SorterData, iSorter, regRecord, iIdx); > sqlite3VdbeAddOp3(v, OP_Last, iIdx, 0, -1); > sqlite3VdbeAddOp2(v, OP_IdxInsert, iIdx, regRecord); > - sqlite3VdbeChangeP5(v, OPFLAG_USESEEKRESULT); > sqlite3ReleaseTempReg(pParse, regRecord); > sqlite3VdbeAddOp2(v, OP_SorterNext, iSorter, addr2); > VdbeCoverage(v); > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index c7fe92cd6..c76910e5b 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -851,23 +851,7 @@ sqlite3Insert(Parse * pParse, /* Parser = context */ > ipkColumn >=3D 0, = onError, > endOfLoop, &isReplace, = 0); > sqlite3FkCheck(pParse, pTab, 0, regIns, 0); > - > - /* Set the OPFLAG_USESEEKRESULT flag if either (a) there = are no REPLACE > - * constraints or (b) there are no triggers and this = table is not a > - * parent table in a foreign key constraint. It is safe = to set the > - * flag in the second case as if any REPLACE constraint = is hit, an > - * OP_Delete or OP_IdxDelete instruction will be = executed on each > - * cursor that is disturbed. And these instructions both = clear the > - * VdbeCursor.seekResult variable, disabling the = OPFLAG_USESEEKRESULT > - * functionality. > - */ > - bool no_foreign_keys =3D > - sqlite3FkReferences(pTab) =3D=3D NULL || > - (user_session->sql_flags & SQLITE_ForeignKeys) = =3D=3D 0; > - bool use_seek =3D isReplace =3D=3D 0 || > - (pTrigger =3D=3D NULL && = no_foreign_keys); > - vdbe_emit_complete_insertion(v, iIdxCur, aRegIdx[0], = use_seek, > - onError); > + vdbe_emit_insertion_completion(v, iIdxCur, aRegIdx[0], = onError); > } > /* Update the count of rows that are inserted > @@ -1491,20 +1475,16 @@ sqlite3GenerateConstraintChecks(Parse * = pParse, /* The parser context */ > } > void > -vdbe_emit_complete_insertion(Vdbe *v, int cursor_id, int tuple_id, > - bool use_seek_result, u8 on_error) > +vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id, = u8 on_error) > { > assert(v !=3D NULL); > - u16 pik_flags =3D OPFLAG_NCHANGE; > - if (use_seek_result) > - pik_flags |=3D OPFLAG_USESEEKRESULT; > - > int opcode; > if (on_error =3D=3D ON_CONFLICT_ACTION_REPLACE) > opcode =3D OP_IdxReplace; > else > opcode =3D OP_IdxInsert; > + u16 pik_flags =3D OPFLAG_NCHANGE; > if (on_error =3D=3D ON_CONFLICT_ACTION_IGNORE) > pik_flags |=3D OPFLAG_OE_IGNORE; > else if (on_error =3D=3D ON_CONFLICT_ACTION_FAIL) > @@ -1910,7 +1890,6 @@ xferOptimization(Parse * pParse, /* = Parser context */ > } > for (pDestIdx =3D pDest->pIndex; pDestIdx; pDestIdx =3D = pDestIdx->pNext) { > - u8 idxInsFlags =3D 0; > for (pSrcIdx =3D pSrc->pIndex; ALWAYS(pSrcIdx); > pSrcIdx =3D pSrcIdx->pNext) { > if (xferCompatibleIndex(pDestIdx, pSrcIdx)) > @@ -1927,11 +1906,9 @@ xferOptimization(Parse * pParse, /* = Parser context */ > addr1 =3D sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0); > VdbeCoverage(v); > sqlite3VdbeAddOp2(v, OP_RowData, iSrc, regData); > - if (pDestIdx->idxType =3D=3D SQLITE_IDXTYPE_PRIMARYKEY) = { > - idxInsFlags |=3D OPFLAG_NCHANGE; > - } > sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData); > - sqlite3VdbeChangeP5(v, idxInsFlags | OPFLAG_APPEND); > + if (pDestIdx->idxType =3D=3D SQLITE_IDXTYPE_PRIMARYKEY) Nitpicking: you can use IsPimaryKeyIndex() macros for this purpose. > + sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); > sqlite3VdbeAddOp2(v, OP_Next, iSrc, addr1 + 1); > VdbeCoverage(v); > sqlite3VdbeJumpHere(v, addr1); > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 95bd7d656..9e64b434e 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -1151,7 +1151,6 @@ selectInnerLoop(Parse * pParse, /* The = parser context */ > if (eDest =3D=3D SRT_DistQueue) { > sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm = + 1, > r3); > - sqlite3VdbeChangeP5(v, = OPFLAG_USESEEKRESULT); > } > for (i =3D 0; i < nKey; i++) { > sqlite3VdbeAddOp2(v, OP_SCopy, > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index e786cf842..7c0a214b5 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -3039,8 +3039,6 @@ struct Parse { > /* Also used in P2 (not P5) of = OP_Delete */ > #define OPFLAG_EPHEM 0x01 /* OP_Column: Ephemeral output = is ok */ > #define OPFLAG_ISUPDATE 0x04 /* This OP_Insert is an sql = UPDATE */ > -#define OPFLAG_APPEND 0x08 /* This is likely to be an = append */ > -#define OPFLAG_USESEEKRESULT 0x10 /* Try to avoid a seek in = BtreeInsert() */ > #define OPFLAG_OE_IGNORE 0x200 /* OP_IdxInsert: Ignore flag */ > #define OPFLAG_OE_FAIL 0x400 /* OP_IdxInsert: Fail flag */ > #ifdef SQLITE_ENABLE_PREUPDATE_HOOK > @@ -3722,13 +3720,11 @@ void sqlite3GenerateConstraintChecks(Parse *, = Table *, int *, int, int, int, > * @param v Virtual database engine. > * @param cursor_id Primary index cursor. > * @param tuple_id Register with data to insert. > - * @param use_seek_result True to set the USESEEKRESULT flag on > - * OP_[Idx]Insert. > * @param on_error Error action on failed insert/replace. > */ > void > -vdbe_emit_complete_insertion(Vdbe *v, int cursor_id, int tuple_id, > - bool use_seek_result, u8 on_error); > +vdbe_emit_insertion_completion(Vdbe *v, int cursor_id, int tuple_id, > + u8 on_error); > int sqlite3OpenTableAndIndices(Parse *, Table *, int, u8, int, u8 *, = int *, > int *, u8, u8); > diff --git a/src/box/sql/update.c b/src/box/sql/update.c > index 859189a7e..98e696ddf 100644 > --- a/src/box/sql/update.c > +++ b/src/box/sql/update.c > @@ -615,8 +615,7 @@ sqlite3Update(Parse * pParse, /* The = parser context */ > } > /* Insert the new index entries and the new record. */ > - vdbe_emit_complete_insertion(v, iIdxCur, aRegIdx[0], = false, > - onError); > + vdbe_emit_insertion_completion(v, iIdxCur, aRegIdx[0], = onError); > /* 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/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index bf8a27709..5a56727da 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -4321,24 +4321,11 @@ case OP_Next: /* jump */ > /* Opcode: IdxInsert P1 P2 * * P5 > * Synopsis: key=3Dr[P2] > * > - * Register P2 holds an SQL index key made using the > - * MakeRecord instructions. This opcode writes that key > - * into the index P1. Data for the entry is nil. > - * > - * If P5 has the OPFLAG_APPEND bit set, that is a hint to the b-tree = layer > - * that this insert is likely to be an append. > - * > - * If P5 has the OPFLAG_NCHANGE bit set, then the change counter is > - * incremented by this instruction. If the OPFLAG_NCHANGE bit is = clear, > - * then the change counter is unchanged. > - * > - * If the OPFLAG_USESEEKRESULT flag of P5 is set, the implementation = might > - * run faster by avoiding an unnecessary seek on cursor P1. However, > - * the OPFLAG_USESEEKRESULT flag must only be set if there have been = no prior > - * seeks on the cursor or if the most recent seek used a key = equivalent > - * to P2. > - * > - * This instruction only works for indices. > + * @param P1 Index of a space cursor. > + * @param P2 Index of a register with MessagePack data to insert. > + * @param P5 Flags. If P5 contains OPFLAG_NCHANGE, then VDBE > + * accounts the change in a case of successful insertion in > + * nChange counter. > */ > /* Opcode: IdxReplace P1 P2 * * P5 > * Synopsis: key=3Dr[P2] > diff --git a/src/box/sql/where.c b/src/box/sql/where.c > index 79de5d69a..c51bd7455 100644 > --- a/src/box/sql/where.c > +++ b/src/box/sql/where.c > @@ -844,7 +844,6 @@ constructAutomaticIndex(Parse * pParse, = /* The parsing context */ > sqlite3GenerateIndexKey(pParse, pIdx, pLevel->iTabCur, = regRecord, 0, > 0, 0); > sqlite3VdbeAddOp2(v, OP_IdxInsert, pLevel->iIdxCur, regRecord); > - sqlite3VdbeChangeP5(v, OPFLAG_USESEEKRESULT); > if (pPartial) > sqlite3VdbeResolveLabel(v, iContinue); > if (pTabItem->fg.viaCoroutine) { > diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c > index a3c51eef4..aaffa7c1c 100644 > --- a/src/box/sql/wherecode.c > +++ b/src/box/sql/wherecode.c > @@ -1766,10 +1766,6 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * = pWInfo, /* Complete information about t > = sqlite3VdbeAddOp2 > (v, = OP_IdxInsert, > = regRowset, regPk); > - if (iSet) > - = sqlite3VdbeChangeP5 > - = (v, > - = OPFLAG_USESEEKRESULT); > } > /* Release the array of = temp registers */ >=20 >=20 >=20