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 4421D2B8D3 for ; Wed, 4 Apr 2018 06:26: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 1otxS6BIHg0z for ; Wed, 4 Apr 2018 06:26:09 -0400 (EDT) Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (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 640E8251F6 for ; Wed, 4 Apr 2018 06:26:08 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 1/4] sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace] References: <2b18eeeaf3c68c8c33de3dd7c8e59eac10b3bd8c.1522769319.git.v.shpilevoy@tarantool.org> From: Vladislav Shpilevoy Message-ID: <990a2f61-37b2-e986-8986-9ef2d90781b0@tarantool.org> Date: Wed, 4 Apr 2018 13:26:00 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US 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, "n.pettik" 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’, > 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. > >> + bool no_foreign_keys = >> + sqlite3FkReferences(pTab) == NULL || >> + (user_session->sql_flags & SQLITE_ForeignKeys) == 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. In any case, talking about the patch, I found a way to remove this code completely. See diff below. 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 >= 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 = - sqlite3FkReferences(pTab) == NULL || - (user_session->sql_flags & SQLITE_ForeignKeys) == 0; - bool use_seek = isReplace == 0 || - (pTrigger == 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 != NULL); - u16 pik_flags = OPFLAG_NCHANGE; - if (use_seek_result) - pik_flags |= OPFLAG_USESEEKRESULT; - int opcode; if (on_error == ON_CONFLICT_ACTION_REPLACE) opcode = OP_IdxReplace; else opcode = OP_IdxInsert; + u16 pik_flags = OPFLAG_NCHANGE; if (on_error == ON_CONFLICT_ACTION_IGNORE) pik_flags |= OPFLAG_OE_IGNORE; else if (on_error == ON_CONFLICT_ACTION_FAIL) @@ -1910,7 +1890,6 @@ xferOptimization(Parse * pParse, /* Parser context */ } for (pDestIdx = pDest->pIndex; pDestIdx; pDestIdx = pDestIdx->pNext) { - u8 idxInsFlags = 0; for (pSrcIdx = pSrc->pIndex; ALWAYS(pSrcIdx); pSrcIdx = pSrcIdx->pNext) { if (xferCompatibleIndex(pDestIdx, pSrcIdx)) @@ -1927,11 +1906,9 @@ xferOptimization(Parse * pParse, /* Parser context */ addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0); VdbeCoverage(v); sqlite3VdbeAddOp2(v, OP_RowData, iSrc, regData); - if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY) { - idxInsFlags |= OPFLAG_NCHANGE; - } sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData); - sqlite3VdbeChangeP5(v, idxInsFlags | OPFLAG_APPEND); + if (pDestIdx->idxType == SQLITE_IDXTYPE_PRIMARYKEY) + 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 == SRT_DistQueue) { sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm + 1, r3); - sqlite3VdbeChangeP5(v, OPFLAG_USESEEKRESULT); } for (i = 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=r[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=r[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 */