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

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

[-- Attachment #1: Type: text/plain, Size: 11296 bytes --]

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 <mailto: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 */


[-- Attachment #2: Type: text/html, Size: 41149 bytes --]