[tarantool-patches] Re: [PATCH 1/4] sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace]
n.pettik
korablev at tarantool.org
Wed Apr 4 02:01:31 MSK 2018
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 at 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)
>
>
More information about the Tarantool-patches
mailing list