Now the whole patch-set looks OK to me.

On 4 Apr 2018, at 14:51, n.pettik <korablev@tarantool.org> wrote:

You may not pay attention to ’nitpickings’,
but I should make visibility of reviewing this patch.

On 4 Apr 2018, at 13:26, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:

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.

Ok, I have misunderstood you.
Now it is clear that you mentioned another operand.


+ 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.

Ok, looks pretty good.



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)

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 == 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 */