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

["n.pettik" <korablev@tarantool.org>]

Overall, I do like provided refactoring:
somewhere things become much easier.

>sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace]

Not opcodes themselves, but opcode operands.

> On 3 Apr 2018, at 18:34, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> And rework sqlite3CompleteInsertion. Now it

Nitpicking: not *now*, but it was before patch.
It sounds very confusing.

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

Nitpicking: *makes no sense*.

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

> ---
> 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);

Nitpicking: *I doubt that this diff is related to patch,
but codestyle fixes are always appreciated.*

> 		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;

AFAIK SQLITE_ForeignKeys flag is always set to be true
(since we always use FK). Thus, you can remove this condition check.

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

What does mean ‘'Data for the entry is nil.” ?
I know that it could be boring and it is not included to diff,
but what about updating whole commentto this opcode?

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

The same here: I guess this comment is obsolete.

>  *
> @@ -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)
> 
>