[tarantool-patches] [PATCH 1/4] sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace]

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

And rework sqlite3CompleteInsertion. Now it
* accepts actually unused pTab;
* array of records to insert in different indexes, but inserts
  only one;
* has sqlite code style.

Code style fix is obvious. PTab is used only to check, that it is
not a view and to check, that a first index is primary:
1) caller functions already guarantee the pTab is not a view;
2) regardless of a first index nature: primary or not - it is not
   used here. It is useless check. With the same success we can
   check this in each function, that uses struct Table.

Array of records to insert has no sense, since insertion is being
done in a primary index only. It is enough to pass a register of
a primary index.
---
 src/box/sql/analyze.c   |  8 ++---
 src/box/sql/delete.c    |  2 +-
 src/box/sql/expr.c      |  5 ++--
 src/box/sql/insert.c    | 80 ++++++++++++-------------------------------------
 src/box/sql/select.c    | 29 +++++++-----------
 src/box/sql/sqliteInt.h | 16 +++++++++-
 src/box/sql/trigger.c   |  3 +-
 src/box/sql/update.c    |  6 ++--
 src/box/sql/vdbe.c      |  9 ++----
 src/box/sql/wherecode.c |  5 ++--
 10 files changed, 59 insertions(+), 104 deletions(-)

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index fb46641a2..c5dcedd3e 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -1085,11 +1085,9 @@ analyzeOneTable(Parse * pParse,	/* Parser context */
 									 regCol + i);
 		}
 		sqlite3VdbeAddOp3(v, OP_MakeRecord, regCol, nColTest,
-					regSample);
-		sqlite3VdbeAddOp3(v, OP_MakeRecord, regTabname, 6,
-					regTemp);
-		sqlite3VdbeAddOp2(v, OP_IdxReplace, iStatCur + 1,
-					regTemp);
+				  regSample);
+		sqlite3VdbeAddOp3(v, OP_MakeRecord, regTabname, 6, regTemp);
+		sqlite3VdbeAddOp2(v, OP_IdxReplace, iStatCur + 1, regTemp);
 		sqlite3VdbeAddOp2(v, OP_Goto, 1, addrNext);	/* P1==1 for end-of-loop */
 		sqlite3VdbeJumpHere(v, addrIsNull);
 
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 7d1d71577..ab8dd0bc3 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -476,7 +476,7 @@ sqlite3DeleteFrom(Parse * pParse,	/* The parser context */
 			sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, nPk, iKey, zAff, nPk);
 			/* Set flag to save memory allocating one by malloc. */
 			sqlite3VdbeChangeP5(v, 1);
-			sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iEphCur, iKey, iPk, nPk);
+			sqlite3VdbeAddOp2(v, OP_IdxInsert, iEphCur, iKey);
 		}
 
 		/* If this DELETE cannot use the ONEPASS strategy, this is the
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index f4e3cf735..78ebc91a5 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -2911,9 +2911,8 @@ sqlite3CodeSubselect(Parse * pParse,	/* Parsing context */
 							  1, r2, &affinity, 1);
 					sqlite3ExprCacheAffinityChange(pParse,
 								       r3, 1);
-					sqlite3VdbeAddOp4Int(v, OP_IdxInsert,
-							     pExpr->iTable, r2,
-							     r3, 1);
+					sqlite3VdbeAddOp2(v, OP_IdxInsert,
+							  pExpr->iTable, r2);
 				}
 				sqlite3ReleaseTempReg(pParse, r1);
 				sqlite3ReleaseTempReg(pParse, r2);
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 4a57b23f5..c7fe92cd6 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -744,7 +744,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 		 * with the first column.
 		 */
 		for (i = 0; i < pTab->nCol; i++) {
-			int iRegStore = regTupleid + 1 + i;
+			int iRegStore = regData + i;
 			if (pColumn == 0) {
 				j = i;
 			} else {
@@ -846,7 +846,6 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 		   and do the insertion.
 		 */
 		int isReplace;	/* Set to true if constraints may cause a replace */
-		int bUseSeek;	/* True to use OPFLAG_SEEKRESULT */
 		sqlite3GenerateConstraintChecks(pParse, pTab, aRegIdx, iDataCur,
 						iIdxCur, regIns, 0,
 						ipkColumn >= 0, onError,
@@ -862,12 +861,13 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 		 * VdbeCursor.seekResult variable, disabling the OPFLAG_USESEEKRESULT
 		 * functionality.
 		 */
-		bUseSeek = isReplace == 0 || (pTrigger == 0 &&
-					      ((user_session->sql_flags &
-						SQLITE_ForeignKeys) == 0 ||
-					       sqlite3FkReferences(pTab) == 0));
-		sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx,
-					 bUseSeek, onError);
+		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);
 	}
 
 	/* Update the count of rows that are inserted
@@ -1490,69 +1490,27 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 	VdbeModuleComment((v, "END: GenCnstCks(%d)", seenReplace));
 }
 
-/*
- * This routine generates code to finish the INSERT or UPDATE operation
- * that was started by a prior call to sqlite3GenerateConstraintChecks.
- * A consecutive range of registers starting at regNewData contains the
- * tupleid and the content to be inserted.
- *
- * The arguments to this routine should be the same as corresponding
- * arguments to sqlite3GenerateConstraintChecks.
- */
 void
-sqlite3CompleteInsertion(Parse * pParse,	/* The parser context */
-			 Table * pTab,		/* the table into which we are inserting */
-			 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)
+vdbe_emit_complete_insertion(Vdbe *v, int cursor_id, int tuple_id,
+			     bool use_seek_result, u8 on_error)
 {
-	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;
-
-	v = sqlite3GetVdbe(pParse);
-	assert(v != 0);
-	/* This table is not a VIEW */
-	assert(!space_is_view(pTab));
-	/*
-	 * The for loop which purpose in sqlite was to insert new
-	 * values to all indexes is replaced to inserting new
-	 * values only to pk in tarantool.
-	 */
-	pIdx = pTab->pIndex;
-	/* Each table have pk on top of the indexes list */
-	assert(IsPrimaryKeyIndex(pIdx));
-	/* Partial indexes should be implemented somewhere in tarantool
-	 * codebase to check it during inserting values to the pk #2626
-	 *
-	 */
-	/*if( pIdx->pPartIdxWhere ){
-	 *  sqlite3VdbeAddOp2(v, OP_IsNull, aRegIdx[i], sqlite3VdbeCurrentAddr(v)+2);
-	 *  VdbeCoverage(v);
-	 *}
-	 */
-	pik_flags = OPFLAG_NCHANGE;
-	if (useSeekResult) {
+	assert(v != NULL);
+	u16 pik_flags = OPFLAG_NCHANGE;
+	if (use_seek_result)
 		pik_flags |= OPFLAG_USESEEKRESULT;
-	}
-	assert(pParse->nested == 0);
 
-	if (onError == ON_CONFLICT_ACTION_REPLACE) {
+	int opcode;
+	if (on_error == ON_CONFLICT_ACTION_REPLACE)
 		opcode = OP_IdxReplace;
-	} else {
+	else
 		opcode = OP_IdxInsert;
-	}
 
-	if (onError == ON_CONFLICT_ACTION_IGNORE) {
+	if (on_error == ON_CONFLICT_ACTION_IGNORE)
 		pik_flags |= OPFLAG_OE_IGNORE;
-	} else if (onError == ON_CONFLICT_ACTION_FAIL) {
+	else if (on_error == ON_CONFLICT_ACTION_FAIL)
 		pik_flags |= OPFLAG_OE_FAIL;
-	}
 
-	sqlite3VdbeAddOp4Int(v, opcode, iIdxCur, aRegIdx[0],
-			     aRegIdx[0] + 1, index_column_count(pIdx));
+	sqlite3VdbeAddOp2(v, opcode, cursor_id, tuple_id);
 	sqlite3VdbeChangeP5(v, pik_flags);
 }
 
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index c7f87f340..95bd7d656 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -667,13 +667,11 @@ pushOntoSorter(Parse * pParse,		/* Parser context */
 		sqlite3ExprCodeMove(pParse, regBase, regPrevKey, pSort->nOBSat);
 		sqlite3VdbeJumpHere(v, addrJmp);
 	}
-	if (pSort->sortFlags & SORTFLAG_UseSorter) {
+	if (pSort->sortFlags & SORTFLAG_UseSorter)
 		op = OP_SorterInsert;
-	} else {
+	else
 		op = OP_IdxInsert;
-	}
-	sqlite3VdbeAddOp4Int(v, op, pSort->iECursor, regRecord,
-			     regBase + nOBSat, nBase - nOBSat);
+	sqlite3VdbeAddOp2(v, op, pSort->iECursor, regRecord);
 	if (iLimit) {
 		int addr;
 		int r1 = 0;
@@ -752,7 +750,7 @@ codeDistinct(Parse * pParse,	/* Parsing and code generating context */
 	sqlite3VdbeAddOp4Int(v, OP_Found, iTab, addrRepeat, iMem, N);
 	VdbeCoverage(v);
 	sqlite3VdbeAddOp3(v, OP_MakeRecord, iMem, N, r1);
-	sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iTab, r1, iMem, N);
+	sqlite3VdbeAddOp2(v, OP_IdxInsert, iTab, r1);
 	sqlite3ReleaseTempReg(pParse, r1);
 }
 
@@ -967,8 +965,7 @@ selectInnerLoop(Parse * pParse,		/* The parser context */
 			r1 = sqlite3GetTempReg(pParse);
 			sqlite3VdbeAddOp3(v, OP_MakeRecord, regResult,
 					  nResultCol, r1);
-			sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iParm, r1,
-					     regResult, nResultCol);
+			sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm, r1);
 			sqlite3ReleaseTempReg(pParse, r1);
 			break;
 		}
@@ -1011,8 +1008,8 @@ selectInnerLoop(Parse * pParse,		/* The parser context */
 				sqlite3VdbeAddOp4Int(v, OP_Found, iParm + 1,
 						     addr, r1, 0);
 				VdbeCoverage(v);
-				sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iParm + 1,
-						     r1, regResult, nResultCol);
+				sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm + 1,
+						  r1);
 				assert(pSort == 0);
 			}
 #endif
@@ -1068,8 +1065,7 @@ selectInnerLoop(Parse * pParse,		/* The parser context */
 				sqlite3ExprCacheAffinityChange(pParse,
 							       regResult,
 							       nResultCol);
-				sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iParm, r1,
-						     regResult, nResultCol);
+				sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm, r1);
 				sqlite3ReleaseTempReg(pParse, r1);
 			}
 			break;
@@ -1165,8 +1161,7 @@ selectInnerLoop(Parse * pParse,		/* The parser context */
 			}
 			sqlite3VdbeAddOp2(v, OP_Sequence, iParm, r2 + nKey);
 			sqlite3VdbeAddOp3(v, OP_MakeRecord, r2, nKey + 2, r1);
-			sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iParm, r1, r2,
-					     nKey + 2);
+			sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm, r1);
 			if (addrTest)
 				sqlite3VdbeJumpHere(v, addrTest);
 			sqlite3ReleaseTempReg(pParse, r1);
@@ -1518,8 +1513,7 @@ generateSortTail(Parse * pParse,	/* Parsing context */
 			sqlite3VdbeAddOp4(v, OP_MakeRecord, regRow, nColumn,
 					  regTupleid, pDest->zAffSdst, nColumn);
 			sqlite3ExprCacheAffinityChange(pParse, regRow, nColumn);
-			sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iParm, regTupleid,
-					     regRow, nColumn);
+			sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm, regTupleid);
 			break;
 		}
 	case SRT_Mem:{
@@ -2982,8 +2976,7 @@ generateOutputSubroutine(Parse * pParse,	/* Parsing context */
 					  pIn->nSdst);
 			sqlite3ExprCacheAffinityChange(pParse, pIn->iSdst,
 						       pIn->nSdst);
-			sqlite3VdbeAddOp4Int(v, OP_IdxInsert, pDest->iSDParm,
-					     r1, pIn->iSdst, pIn->nSdst);
+			sqlite3VdbeAddOp2(v, OP_IdxInsert, pDest->iSDParm, r1);
 			sqlite3ReleaseTempReg(pParse, r1);
 			break;
 		}
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index f43711a7f..e786cf842 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3715,7 +3715,21 @@ 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);
+/**
+ * This routine generates code to finish the INSERT or UPDATE
+ * operation that was started by a prior call to
+ * sqlite3GenerateConstraintChecks.
+ * @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);
+
 int sqlite3OpenTableAndIndices(Parse *, Table *, int, u8, int, u8 *, int *,
 			       int *, u8, u8);
 void sqlite3BeginWriteOperation(Parse *, int);
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 859b421db..eaa08eb4c 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -281,8 +281,7 @@ sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
 		sqlite3VdbeAddOp4(v, OP_Blob, zOptsSz, iFirstCol + 1,
 				  MSGPACK_SUBTYPE, zOpts, P4_DYNAMIC);
 		sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 2, iRecord);
-		sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iCursor, iRecord,
-				     iFirstCol, 7);
+		sqlite3VdbeAddOp2(v, OP_IdxInsert, iCursor, iRecord);
 		/* Do not account nested operations: the count of such
 		 * operations depends on Tarantool data dictionary internals,
 		 * such as data layout in system spaces.
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 7727ae5d5..859189a7e 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -377,8 +377,7 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 				   sqlite3IndexAffinityStr(pParse->db, pPk);
 		sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, nPk, regKey,
 					  zAff, nPk);
-		sqlite3VdbeAddOp4Int(v, OP_IdxInsert, iEph, regKey, iPk,
-				     nPk);
+		sqlite3VdbeAddOp2(v, OP_IdxInsert, iEph, regKey);
 		/* Set flag to save memory allocating one by malloc. */
 		sqlite3VdbeChangeP5(v, 1);
 	}
@@ -616,7 +615,8 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 		}
 
 		/* Insert the new index entries and the new record. */
-		sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx, 0, onError);
+		vdbe_emit_complete_insertion(v, iIdxCur, aRegIdx[0], false,
+					     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 329d46886..bf8a27709 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4318,18 +4318,13 @@ case OP_Next:          /* jump */
 	goto check_for_interrupt;
 }
 
-/* Opcode: IdxInsert P1 P2 P3 P4 P5
+/* 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 P4 is not zero, then it is the number of values in the unpacked
- * key of reg(P2).  In that case, P3 is the index of the first register
- * for the unpacked key.  The availability of the unpacked key can sometimes
- * be an optimization.
- *
  * 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.
  *
@@ -4345,7 +4340,7 @@ case OP_Next:          /* jump */
  *
  * This instruction only works for indices.
  */
-/* Opcode: IdxReplace P1 P2 P3 P4 P5
+/* Opcode: IdxReplace P1 P2 * * P5
  * Synopsis: key=r[P2]
  *
  * This opcode works exactly as IdxInsert does, but in Tarantool
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 8b06baeff..a3c51eef4 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1763,10 +1763,9 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about t
 							sqlite3VdbeAddOp3
 								(v, OP_MakeRecord,
 								 r, nPk, regPk);
-							sqlite3VdbeAddOp4Int
+							sqlite3VdbeAddOp2
 								(v, OP_IdxInsert,
-								 regRowset, regPk,
-								 r, nPk);
+								 regRowset, regPk);
 							if (iSet)
 								sqlite3VdbeChangeP5
 									(v,
-- 
2.14.3 (Apple Git-98)