[tarantool-patches] Re: [PATCH 1/4] sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace]
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Apr 4 13:26:00 MSK 2018
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 */
More information about the Tarantool-patches
mailing list