From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 12F012C0F5 for ; Tue, 3 Apr 2018 19:01:34 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id LbzBLoK1yd_D for ; Tue, 3 Apr 2018 19:01:33 -0400 (EDT) Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 9FED72C0B9 for ; Tue, 3 Apr 2018 19:01:33 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH 1/4] sql: remove unused opcodes from OP_[Sorter/Idx][Insert/Replace] From: "n.pettik" In-Reply-To: <2b18eeeaf3c68c8c33de3dd7c8e59eac10b3bd8c.1522769319.git.v.shpilevoy@tarantool.org> Date: Wed, 4 Apr 2018 02:01:31 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <2b18eeeaf3c68c8c33de3dd7c8e59eac10b3bd8c.1522769319.git.v.shpilevoy@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Vladislav Shpilevoy Cc: tarantool-patches@freelists.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 = wrote: >=20 > 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. >=20 > 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. >=20 > 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=E2=80=99, 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(-) >=20 > 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=3D=3D= 1 for end-of-loop */ > sqlite3VdbeJumpHere(v, addrIsNull); >=20 > 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); > } >=20 > /* 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 =3D 0; i < pTab->nCol; i++) { > - int iRegStore =3D regTupleid + 1 + i; > + int iRegStore =3D regData + i; > if (pColumn =3D=3D 0) { > j =3D 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 >=3D 0, = onError, > @@ -862,12 +861,13 @@ sqlite3Insert(Parse * pParse, /* Parser = context */ > * VdbeCursor.seekResult variable, disabling the = OPFLAG_USESEEKRESULT > * functionality. > */ > - bUseSeek =3D isReplace =3D=3D 0 || (pTrigger =3D=3D 0 && > - ((user_session->sql_flags = & > - SQLITE_ForeignKeys) =3D=3D= 0 || > - sqlite3FkReferences(pTab) = =3D=3D 0)); > - sqlite3CompleteInsertion(pParse, pTab, iIdxCur, aRegIdx, > - bUseSeek, onError); > + bool no_foreign_keys =3D > + sqlite3FkReferences(pTab) =3D=3D NULL || > + (user_session->sql_flags & SQLITE_ForeignKeys) = =3D=3D 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 =3D isReplace =3D=3D 0 || > + (pTrigger =3D=3D NULL && = no_foreign_keys); > + vdbe_emit_complete_insertion(v, iIdxCur, aRegIdx[0], = use_seek, > + onError); > } >=20 > /* Update the count of rows that are inserted > @@ -1490,69 +1490,27 @@ sqlite3GenerateConstraintChecks(Parse * = pParse, /* The parser context */ > VdbeModuleComment((v, "END: GenCnstCks(%d)", seenReplace)); > } >=20 > -/* > - * 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 =3D sqlite3GetVdbe(pParse); > - assert(v !=3D 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 =3D 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 =3D OPFLAG_NCHANGE; > - if (useSeekResult) { > + assert(v !=3D NULL); > + u16 pik_flags =3D OPFLAG_NCHANGE; > + if (use_seek_result) > pik_flags |=3D OPFLAG_USESEEKRESULT; > - } > - assert(pParse->nested =3D=3D 0); >=20 > - if (onError =3D=3D ON_CONFLICT_ACTION_REPLACE) { > + int opcode; > + if (on_error =3D=3D ON_CONFLICT_ACTION_REPLACE) > opcode =3D OP_IdxReplace; > - } else { > + else > opcode =3D OP_IdxInsert; > - } >=20 > - if (onError =3D=3D ON_CONFLICT_ACTION_IGNORE) { > + if (on_error =3D=3D ON_CONFLICT_ACTION_IGNORE) > pik_flags |=3D OPFLAG_OE_IGNORE; > - } else if (onError =3D=3D ON_CONFLICT_ACTION_FAIL) { > + else if (on_error =3D=3D ON_CONFLICT_ACTION_FAIL) > pik_flags |=3D OPFLAG_OE_FAIL; > - } >=20 > - sqlite3VdbeAddOp4Int(v, opcode, iIdxCur, aRegIdx[0], > - aRegIdx[0] + 1, index_column_count(pIdx)); > + sqlite3VdbeAddOp2(v, opcode, cursor_id, tuple_id); > sqlite3VdbeChangeP5(v, pik_flags); > } >=20 > 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 =3D OP_SorterInsert; > - } else { > + else > op =3D OP_IdxInsert; > - } > - sqlite3VdbeAddOp4Int(v, op, pSort->iECursor, regRecord, > - regBase + nOBSat, nBase - nOBSat); > + sqlite3VdbeAddOp2(v, op, pSort->iECursor, regRecord); > if (iLimit) { > int addr; > int r1 =3D 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); > } >=20 > @@ -967,8 +965,7 @@ selectInnerLoop(Parse * pParse, /* The = parser context */ > r1 =3D 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 =3D=3D 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 */ > } >=20 > /* 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); >=20 > /* 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; > } >=20 > -/* Opcode: IdxInsert P1 P2 P3 P4 P5 > +/* Opcode: IdxInsert P1 P2 * * P5 > * Synopsis: key=3Dr[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 =E2=80=98'Data for the entry is nil.=E2=80=9D ? 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=3Dr[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, > --=20 > 2.14.3 (Apple Git-98) >=20 >=20