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 269A22A0EE for ; Tue, 2 Oct 2018 07:56:59 -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 lo-vXQD2f8Yr for ; Tue, 2 Oct 2018 07:56:59 -0400 (EDT) Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 43E3C2A0E8 for ; Tue, 2 Oct 2018 07:56:58 -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] sql: refactor SQL cursor to remove write ones From: "n.pettik" In-Reply-To: <4aec5b52bc28f47ad2af0ac6f4e64be0712f378b.1538389679.git.kyukhin@tarantool.org> Date: Tue, 2 Oct 2018 14:56:54 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <7816B3E4-321F-446F-B19A-ED3E241D5D86@tarantool.org> References: <4aec5b52bc28f47ad2af0ac6f4e64be0712f378b.1538389679.git.kyukhin@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: tarantool-patches@freelists.org Cc: Kirill Yukhin Firstly, I see that code on branch differs (at the moment of writing = review) from that you send to review (in this letter). Outdated version contains a lot of = artefacts (tarantoolSqlite3EphemeralCreate2(), OP_IdxReplace2 etc). Please, push your ready-for-review version. In general patch is OK, I have spotted only several nits. Also, I support your intention (which we discussed outside mailing list) = to make system spaces be =E2=80=98pre-loaded=E2=80=99. It means that all = required system spaces will be stored in predefined (at the start of program) registers. Moreover, I = suggest to attempt at involving not only system spaces. Idea is following: each OP_CursorOpen during compilation stage skips register where space should be stored. At the end of compilation (sql_finish_coding) we know = that totally program uses n registers. Then, we can allocate n + m (where m = is number of distinct spaces which are used in out query) registers to hold = pointers to required spaces. Finally, we patch each OP_CursorOpen and substitute void arg = with number of register where required space is stored. This approach for = instance allows to update all space pointers after schema changes with ease. Finally, now I think about renaming CursorOpen to IteratorOpen: in fact = after your patch cursor has turned almost in wrapper around index iterator. Such = naming would underline the fact that =E2=80=98cursor=E2=80=99 is used only for = read iterations over the space. > On 1 Oct 2018, at 13:31, Kirill Yukhin wrote: >=20 > In Tarantool opening and positioning cursor for writing > have no sense. So refactor SQL code, to perform: > - Creation of ephemeral table w/o any cursors machinery. > No op-code returns register which contains plain pointer > to the ephemeral space. > - OpenRead/OpenWrite opcodes were replaced with single > CursorOpen op-code, which establishes new cursor w/ > intention to sebsequent read from the space. This opcode Nit: subsequent > accepts both plain pointer (in P4 operand) and register > which contains pointer (inn P3) to the ephemeral space. > This query scheduler and DML routines thoroughly. Nit: probably you missed verb in this sentence... > diff --git a/src/box/sql.c b/src/box/sql.c > index ab4a587..c5461ad 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -355,31 +355,15 @@ int tarantoolSqlite3Count(BtCursor *pCur, i64 = *pnEntry) > return SQLITE_OK; > } >=20 > -/** > - * Create ephemeral space and set cursor to the first entry. Features = of > - * ephemeral spaces: id =3D=3D 0, name =3D=3D "ephemeral", memtx = engine (in future it > - * can be changed, but now only memtx engine is supported), primary = index > - * which covers all fields and no secondary indexes, given field = number and > - * collation sequence. All fields are scalar and nullable. > - * > - * @param pCur Cursor which will point to the new ephemeral space. > - * @param field_count Number of fields in ephemeral space. > - * @param key_info Keys description for new ephemeral space. > - * > - * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise. > - */ > -int > -tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count, > +struct space * > +tarantoolSqlite3EphemeralCreate(uint32_t field_count, > struct sql_key_info *key_info) > { Would you mind to rename this function according to original Tarantool = codestyle? > int tarantoolSqlite3EphemeralInsert(struct space *space, const char = *tuple, > @@ -1461,35 +1436,31 @@ sql_debug_info(struct info_handler *h) > info_end(h); > } >=20 > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 43be777..0ae4c7d 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -883,7 +883,7 @@ vdbe_emit_open_cursor(struct Parse *parse_context, = int cursor, int index_id, > struct space *space) > { > assert(space !=3D NULL); > - return sqlite3VdbeAddOp4(parse_context->pVdbe, OP_OpenWrite, = cursor, > + return sqlite3VdbeAddOp4(parse_context->pVdbe, OP_CursorOpen, = cursor, > index_id, 0, (void *) space, = P4_SPACEPTR); > } >=20 > @@ -2693,7 +2693,7 @@ sql_create_index(struct Parse *parse, struct = Token *token, > goto exit_create_index; >=20 > sql_set_multi_write(parse, true); > - sqlite3VdbeAddOp4(vdbe, OP_OpenWrite, cursor, 0, 0, > + sqlite3VdbeAddOp4(vdbe, OP_CursorOpen, cursor, 0, 0, > (void *)space_by_id(BOX_INDEX_ID), > P4_SPACEPTR); > sqlite3VdbeChangeP5(vdbe, OPFLAG_SEEKEQ); > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index 9aa7058..ebde698 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -69,7 +69,7 @@ sql_lookup_table(struct Parse *parse, struct = SrcList_item *tbl_name) >=20 > void > sql_materialize_view(struct Parse *parse, const char *name, struct = Expr *where, > - int cursor) > + int cursor, int reg_eph) This function is used twice: to materialize view during firing INSTEAD = OF DELETE and INSTEAD OF UPDATE triggers. In both cases, you pass reg_eph which is not used after this function invocation. So I guess you can remove = reg_eph arg. > { > struct sqlite3 *db =3D parse->db; > where =3D sqlite3ExprDup(db, where, 0); > @@ -83,7 +83,7 @@ sql_materialize_view(struct Parse *parse, const char = *name, struct Expr *where, > struct Select *select =3D sqlite3SelectNew(parse, NULL, from, = where, NULL, > NULL, NULL, 0, NULL, = NULL); > struct SelectDest dest; > - sqlite3SelectDestInit(&dest, SRT_EphemTab, cursor); > + sqlite3SelectDestInit(&dest, SRT_EphemTab, cursor, reg_eph); > sqlite3Select(parse, select, &dest); > sql_select_delete(db, select); > } > @@ -194,9 +194,10 @@ sql_table_delete_from(struct Parse *parse, struct = SrcList *tab_list, > /* If we are trying to delete from a view, realize that > * view into an ephemeral table. > */ > + int reg_eph =3D ++parse->nMem; This is what I am talking above: this variable is used once (right = below). > if (is_view) { > sql_materialize_view(parse, space->def->name, where, > - tab_cursor); > + tab_cursor, reg_eph); > } >=20 > /* Initialize the counter of the number of rows deleted, > @@ -390,6 +393,8 @@ sql_table_delete_from(struct Parse *parse, struct = SrcList *tab_list, >=20 > VdbeCoverage(v); > } else { > + sqlite3VdbeAddOp3(v, OP_CursorOpen, > + eph_cursor, 0, = reg_eph); Broken indentation. > addr_loop =3D sqlite3VdbeAddOp1(v, OP_Rewind, = eph_cursor); > VdbeCoverage(v); > sqlite3VdbeAddOp2(v, OP_RowData, eph_cursor, = reg_key); > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index a13de4f..aa26342 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -2714,8 +2714,10 @@ sqlite3CodeSubselect(Parse * pParse, /* = Parsing context */ > */ > pExpr->iTable =3D pParse->nTab++; > pExpr->is_ephemeral =3D 1; > + int reg_eph =3D ++pParse->nMem; > addr =3D sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, > - pExpr->iTable, nVal); > + reg_eph, nVal); > + sqlite3VdbeAddOp3(v, OP_CursorOpen, = pExpr->iTable, 0, reg_eph); Out of 80 chars. > struct sql_key_info *key_info =3D = sql_key_info_new(pParse->db, nVal); > if (key_info =3D=3D NULL) > return 0; > @@ -2736,7 +2738,7 @@ sqlite3CodeSubselect(Parse * pParse, /* = Parsing context */ > SelectDest dest; > int i; > sqlite3SelectDestInit(&dest, = SRT_Set, > - = pExpr->iTable); > + = pExpr->iTable, reg_eph); Out of 80. > dest.zAffSdst =3D > exprINAffinity(pParse, = pExpr); > assert((pExpr->iTable & = 0x0000FFFF) =3D=3D > @@ -2808,8 +2810,10 @@ sqlite3CodeSubselect(Parse * pParse, /* = Parsing context */ > 1, r2, = &affinity, 1); > = sqlite3ExprCacheAffinityChange(pParse, > = r3, 1); > - sqlite3VdbeAddOp2(v, = OP_IdxInsert, > - pExpr->iTable, = r2); > + sqlite3VdbeAddOp2(v, = OP_IdxInsert, r2, > + reg_eph); > + sqlite3VdbeChangeP5(v, > + = OPFLAG_EPH_INSERT); > } > sqlite3ReleaseTempReg(pParse, r1); > sqlite3ReleaseTempReg(pParse, r2); > @@ -2847,7 +2851,7 @@ sqlite3CodeSubselect(Parse * pParse, /* = Parsing context */ >=20 > pSel =3D pExpr->x.pSelect; > nReg =3D pExpr->op =3D=3D TK_SELECT ? = pSel->pEList->nExpr : 1; > - sqlite3SelectDestInit(&dest, 0, pParse->nMem + = 1); > + sqlite3SelectDestInit(&dest, 0, pParse->nMem + = 1, 0); If you don=E2=80=99t use ephemeral register, mb it is worth initialising = it with -1? 0 is still valid register number. > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 03f4e1b..c9b4846 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -46,12 +46,10 @@ > void > sqlite3OpenTable(Parse * pParse, /* Generate code into this VDBE = */ > int iCur, /* The cursor number of the table */ > - struct space *space, /* The table to be opened */ > - int opcode) /* OP_OpenRead or OP_OpenWrite */ > + struct space *space) /* The table to be opened */ > { This function appears only once in source code. Lets simply inline it, instead of refactoring. >=20 > @@ -1327,7 +1334,8 @@ xferOptimization(Parse * pParse, /* = Parser context */ > sqlite3VdbeChangeP5(v, OPFLAG_XFER_OPT); > #endif >=20 > - sqlite3VdbeAddOp2(v, OP_IdxInsert, iDest, regData); > + sqlite3VdbeAddOp4(v, OP_IdxInsert, regData, 0, 0, > + (char *)dest, P4_SPACEPTR); > switch (onError) { > case ON_CONFLICT_ACTION_IGNORE: > sqlite3VdbeChangeP5(v, OPFLAG_OE_IGNORE | = OPFLAG_NCHANGE); > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y > index 473cf89..6b5f19d 100644 > --- a/src/box/sql/parse.y > +++ b/src/box/sql/parse.y > @@ -390,7 +390,7 @@ cmd ::=3D DROP VIEW ifexists(E) fullname(X). { > //////////////////////// The SELECT statement = ///////////////////////////////// > // > cmd ::=3D select(X). { > - SelectDest dest =3D {SRT_Output, 0, 0, 0, 0, 0}; > + SelectDest dest =3D {SRT_Output, 0, 0, 0, 0, 0, 0}; Nit: don=E2=80=99t mess tabs with spaces: in parse.y spaces are used. Yep, it contradicts our codestyle, but lets better make code look uniformly, than messing code styles... > if(!pParse->parse_only) > sqlite3Select(pParse, X, &dest); > else > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 02c0a5d..0370327 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -66,6 +66,8 @@ struct DistinctCtx { > u8 eTnctType; /* One of the WHERE_DISTINCT_* operators = */ > int tabTnct; /* Ephemeral table used for DISTINCT = processing */ > int addrTnct; /* Address of OP_OpenEphemeral opcode = for tabTnct */ > + /* Register, containing a pointer to ephemeral space. */ > + int reg_eph; Please, leave explanation comment; otherwise it seems quite easy to mess tabTnct (which is really cursor used to iterate over ephemeral = space) with reg_eph which in turn is register containing space *. You may explain it one place and refer to it in other. Such schema (I mean ephemeral space * + ephemeral cursor) appears quite frequently. > @@ -831,24 +838,32 @@ codeOffset(Vdbe * v, /* Generate code = into this VM */ > * > * A jump to addrRepeat is made and the N+1 values are popped from the > * stack if the top N elements are not distinct. > + * > + * @param parse Parsing and code generating context. > + * @param cursor A sorting index cursor used to test for = distinctness. > + * @param reg_eph Register holding ephemeral's space pointer. > + * @param addr_repeat Jump to here if not distinct. > + * @param n Number of elements. > + * @param reg_data First register holding the data. > */ > static void > -codeDistinct(Parse * pParse, /* Parsing and code generating context = */ > - int iTab, /* A sorting index used to test for = distinctness */ > - int addrRepeat, /* Jump to here if not distinct */ > - int N, /* Number of elements */ > - int iMem) /* First element */ > +codeDistinct(struct Parse * parse, > + int cursor, > + int reg_eph, > + int addr_repeat, > + int n, > + int reg_data) > { > - Vdbe *v; > + struct Vdbe *v =3D parse->pVdbe; > int r1; >=20 > - v =3D pParse->pVdbe; > - r1 =3D sqlite3GetTempReg(pParse); > - sqlite3VdbeAddOp4Int(v, OP_Found, iTab, addrRepeat, iMem, N); > + r1 =3D sqlite3GetTempReg(parse); > + sqlite3VdbeAddOp4Int(v, OP_Found, cursor, addr_repeat, reg_data, = n); > VdbeCoverage(v); > - sqlite3VdbeAddOp3(v, OP_MakeRecord, iMem, N, r1); > - sqlite3VdbeAddOp2(v, OP_IdxInsert, iTab, r1); > - sqlite3ReleaseTempReg(pParse, r1); > + sqlite3VdbeAddOp3(v, OP_MakeRecord, reg_data, n, r1); > + sqlite3VdbeAddOp2(v, OP_IdxInsert, r1, reg_eph); > + sqlite3VdbeChangeP5(v, OPFLAG_EPH_INSERT); > + sqlite3ReleaseTempReg(parse, r1); > } I slightly refactored this function (you forgot to format arguments, = update comment etc). It is only cosmetic change tho: diff --git a/src/box/sql/select.c b/src/box/sql/select.c index b70b85f54..e6b8b3864 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -831,35 +831,29 @@ codeOffset(Vdbe * v, /* Generate code = into this VM */ } =20 /** - * Add code that will check to make sure the N registers starting at = iMem - * form a distinct entry. iTab is a sorting index that holds = previously - * seen combinations of the N values. A new entry is made in iTab - * if the current N values are new. + * Add code that will check to make sure the @n registers starting + * at @reg_data form a distinct entry. @cursor is a sorting index + * that holds previously seen combinations of the @n values. + * A new entry is made in @cursor if the current n values are new. * - * A jump to addrRepeat is made and the N+1 values are popped from the - * stack if the top N elements are not distinct. + * A jump to @addr_repeat is made and the @n+1 values are popped + * from the stack if the top n elements are not distinct. * * @param parse Parsing and code generating context. - * @param cursor A sorting index cursor used to test for distinctness. - * @param reg_eph Register holding ephemeral's space pointer. - * @param addr_repeat Jump to here if not distinct. - * @param n Number of elements. + * @param cursor A sorting index cursor used to test for + * distinctness. + * @param reg_eph Register holding ephemeral space's pointer. + * @param addr_repeat Jump here if not distinct. + * @param n Number of elements in record. * @param reg_data First register holding the data. */ static void -codeDistinct(struct Parse * parse, - int cursor, - int reg_eph, - int addr_repeat, - int n, - int reg_data) +vdbe_insert_distinct(struct Parse *parse, int cursor, int reg_eph, + int addr_repeat, int n, int reg_data) { struct Vdbe *v =3D parse->pVdbe; - int r1; - - r1 =3D sqlite3GetTempReg(parse); + int r1 =3D sqlite3GetTempReg(parse); sqlite3VdbeAddOp4Int(v, OP_Found, cursor, addr_repeat, reg_data, = n); - VdbeCoverage(v); sqlite3VdbeAddOp3(v, OP_MakeRecord, reg_data, n, r1); sqlite3VdbeAddOp2(v, OP_IdxInsert, r1, reg_eph); sqlite3VdbeChangeP5(v, OPFLAG_EPH_INSERT); @@ -1061,12 +1055,13 @@ selectInnerLoop(Parse * pParse, /* The = parser context */ } =20 default:{ - assert(pDistinct->eTnctType =3D=3D - WHERE_DISTINCT_UNORDERED); - codeDistinct(pParse, pDistinct->tabTnct, = pDistinct->reg_eph, - iContinue, nResultCol, = regResult); - break; - } + assert(pDistinct->eTnctType =3D=3D + WHERE_DISTINCT_UNORDERED); + vdbe_insert_distinct(pParse, pDistinct->tabTnct, + pDistinct->reg_eph, = iContinue, + nResultCol, regResult); + break; + } } if (pSort =3D=3D 0) { codeOffset(v, p->iOffset, iContinue); @@ -5391,8 +5386,8 @@ updateAccumulator(Parse * pParse, AggInfo * = pAggInfo) addrNext =3D sqlite3VdbeMakeLabel(v); testcase(nArg =3D=3D 0); /* Error condition = */ testcase(nArg > 1); /* Also an error */ - codeDistinct(pParse, pF->iDistinct, pF->reg_eph, - addrNext, 1, regAgg); + vdbe_insert_distinct(pParse, pF->iDistinct, = pF->reg_eph, + addrNext, 1, regAgg); >=20 > /* > @@ -998,7 +1013,8 @@ selectInnerLoop(Parse * pParse, /* The = parser context */ > * row is all NULLs. > */ > sqlite3VdbeChangeToNoop(v, = pDistinct->addrTnct); > - pOp =3D sqlite3VdbeGetOp(v, = pDistinct->addrTnct); > + sqlite3VdbeChangeToNoop(v, = pDistinct->addrTnct + 1); > + pOp =3D sqlite3VdbeGetOp(v, = pDistinct->addrTnct + 1); Actually, I don=E2=80=99t understand why you need manipulations with = addrTnct + 1. If they are really vital, then leave explanation comment pls. (I guess it is required since two opcodes (OpenEphemeral + OpenCursor) turn into one (OpenCursor), but I didn=E2=80=99t dive into code). > pOp->opcode =3D OP_Null; > pOp->p1 =3D 1; > pOp->p2 =3D regPrev; > @@ -1040,13 +1056,14 @@ selectInnerLoop(Parse * pParse, = /* The parser context */ >=20 > case WHERE_DISTINCT_UNIQUE:{ > sqlite3VdbeChangeToNoop(v, = pDistinct->addrTnct); > + sqlite3VdbeChangeToNoop(v, = pDistinct->addrTnct + 1); The same is here. Also, out of 80. >=20 > @@ -3417,8 +3459,8 @@ multiSelectOrderBy(Parse * pParse, /* = Parsing context */ > regAddrB =3D ++pParse->nMem; > regOutA =3D ++pParse->nMem; > regOutB =3D ++pParse->nMem; > - sqlite3SelectDestInit(&destA, SRT_Coroutine, regAddrA); > - sqlite3SelectDestInit(&destB, SRT_Coroutine, regAddrB); > + sqlite3SelectDestInit(&destA, SRT_Coroutine, regAddrA, 0); > + sqlite3SelectDestInit(&destB, SRT_Coroutine, regAddrB, 0); Lets initialise unused ephemeral register with -1 here and in other places as well. >=20 > /* Generate a coroutine to evaluate the SELECT statement to the > * left of the compound operator - the "A" select. > @@ -5261,7 +5303,7 @@ resetAccumulator(Parse * pParse, AggInfo * = pAggInfo) > /* Verify that all AggInfo registers are within the range = specified by > * AggInfo.mnReg..AggInfo.mxReg > */ > - assert(nReg =3D=3D pAggInfo->mxReg - pAggInfo->mnReg + 1); > + assert(nReg <=3D pAggInfo->mxReg - pAggInfo->mnReg + 1); Hm? Explain pls this change. > for (i =3D 0; i < pAggInfo->nColumn; i++) { > assert(pAggInfo->aCol[i].iMem >=3D pAggInfo->mnReg > && pAggInfo->aCol[i].iMem <=3D pAggInfo->mxReg); >=20 > @@ -5840,13 +5891,16 @@ sqlite3Select(Parse * pParse, /* The = parser context */ > */ > if (p->selFlags & SF_Distinct) { > sDistinct.tabTnct =3D pParse->nTab++; > + sDistinct.reg_eph =3D ++pParse->nMem; > struct sql_key_info *key_info =3D > sql_expr_list_to_key_info(pParse, p->pEList, 0); > sDistinct.addrTnct =3D sqlite3VdbeAddOp4(v, = OP_OpenTEphemeral, > - = sDistinct.tabTnct, > + = sDistinct.reg_eph, > = key_info->part_count, > 0, (char = *)key_info, > P4_KEYINFO); > + sqlite3VdbeAddOp3(v, OP_CursorOpen, sDistinct.tabTnct, = 0, > + sDistinct.reg_eph); > VdbeComment((v, "Distinct table")); > sDistinct.eTnctType =3D WHERE_DISTINCT_UNORDERED; > } else { > @@ -5886,6 +5940,7 @@ sqlite3Select(Parse * pParse, /* The = parser context */ > */ > if (sSort.addrSortIndex >=3D 0 && sSort.pOrderBy =3D=3D = 0) { > sqlite3VdbeChangeToNoop(v, sSort.addrSortIndex); > + sqlite3VdbeChangeToNoop(v, sSort.addrSortIndex + = 1); Explain pls why do you need this. > } >=20 > /* Use the standard inner loop. */ > @@ -6130,6 +6185,7 @@ sqlite3Select(Parse * pParse, /* The = parser context */ > ) { > sSort.pOrderBy =3D 0; > sqlite3VdbeChangeToNoop(v, = sSort.addrSortIndex); > + sqlite3VdbeChangeToNoop(v, = sSort.addrSortIndex + 1); The same. > } >=20 > /* Evaluate the current GROUP BY terms and store = in b0, b1, b2... > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 53188e7..51110aa 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -2000,6 +2000,8 @@ struct AggInfo { > FuncDef *pFunc; /* The aggregate function implementation = */ > int iMem; /* Memory location that acts as = accumulator */ > int iDistinct; /* Ephemeral table used to enforce = DISTINCT */ > + /* Register, holding ephemeral's spacepointer. */ > + int reg_eph; > } *aFunc; > int nFunc; /* Number of entries in aFunc[] */ > }; > @@ -2327,6 +2329,8 @@ struct SrcList { > } fg; > u8 iSelectId; /* If pSelect!=3D0, the id of the = sub-select in EQP */ > int iCursor; /* The VDBE cursor number used to access = this table */ > + /* Register, containing pointer to the space. */ > + int reg; Do you really need there this reg? AFAIS it used only in = select.c:5741:11 and select.c:5760:35. Could you suggest workaround to avoid adding member to SrcList? I am asking cause SrcList is widely used through source code = and extra member is unlikely to be suitable. > Expr *pOn; /* The ON clause of a join */ > IdList *pUsing; /* The USING clause of a join */ > Bitmask colUsed; /* Bit N (1< @@ -2591,6 +2595,8 @@ struct SelectDest { > u8 eDest; /* How to dispose of the results. On of = SRT_* above. */ > char *zAffSdst; /* Affinity used when eDest=3D=3DSRT_Set = */ > int iSDParm; /* A parameter used by the eDest = disposal method */ > + /* Register containing ephemeral's space pointer. */ > + int reg_eph; > int iSdst; /* Base register where results are = written */ > int nSdst; /* Number of registers allocated */ > ExprList *pOrderBy; /* Key columns for SRT_Queue and = SRT_DistQueue */ >=20 > /** > diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c > index c6bd15b..271f40a 100644 > --- a/src/box/sql/trigger.c > +++ b/src/box/sql/trigger.c > @@ -694,7 +688,7 @@ codeTriggerProgram(Parse * pParse, /* The = parser context */ > SelectDest sDest; > Select *pSelect =3D > sqlite3SelectDup(db, pStep->pSelect, = 0); > - sqlite3SelectDestInit(&sDest, = SRT_Discard, 0); > + sqlite3SelectDestInit(&sDest, = SRT_Discard, 0, 0); Lets initialise unused ephemeral register with -1. > sqlite3Select(pParse, pSelect, &sDest); > sql_select_delete(db, pSelect); > break; > diff --git a/src/box/sql/update.c b/src/box/sql/update.c > index 730872f..71bd71d 100644 > --- a/src/box/sql/update.c > +++ b/src/box/sql/update.c > @@ -222,13 +222,16 @@ sqlite3Update(Parse * pParse, /* The = parser context */ > * an ephemeral table. > */ > uint32_t pk_part_count; > + struct space *space; > if (is_view) { > - sql_materialize_view(pParse, def->name, pWhere, = pk_cursor); > + int reg_eph =3D ++pParse->nMem; > + sql_materialize_view(pParse, def->name, pWhere, = pk_cursor, reg_eph); > /* Number of columns from SELECT plus ID.*/ > pk_part_count =3D nKey =3D def->field_count + 1; > } else { > - vdbe_emit_open_cursor(pParse, pk_cursor, 0, > - space_by_id(pTab->def->id)); > + space =3D space_by_id(pTab->def->id); You don=E2=80=99t need to fetch space from cache: now real space is stored in pTab->space (see sql_lookup_table()). > + assert(space !=3D NULL); > + vdbe_emit_open_cursor(pParse, pk_cursor, 0, space); > pk_part_count =3D pPk->def->key_def->part_count; > } >=20 > @@ -242,11 +245,12 @@ sqlite3Update(Parse * pParse, /* The = parser context */ > int iPk =3D pParse->nMem + 1; > pParse->nMem +=3D pk_part_count; > regKey =3D ++pParse->nMem; > + int reg_eph =3D ++pParse->nMem; > iEph =3D pParse->nTab++; > sqlite3VdbeAddOp2(v, OP_Null, 0, iPk); >=20 > /* Address of the OpenEphemeral instruction. */ > - int addrOpen =3D sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEph, > + int addrOpen =3D sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, = reg_eph, > pk_part_count); > pWInfo =3D sqlite3WhereBegin(pParse, pTabList, pWhere, 0, 0, > WHERE_ONEPASS_DESIRED, pk_cursor); > @@ -276,9 +280,9 @@ sqlite3Update(Parse * pParse, /* The = parser context */ > = pPk->def); > sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, pk_part_count, > regKey, zAff, pk_part_count); > - sqlite3VdbeAddOp2(v, OP_IdxInsert, iEph, regKey); > + sqlite3VdbeAddOp2(v, OP_IdxInsert, regKey, reg_eph); > /* Set flag to save memory allocating one by malloc. */ > - sqlite3VdbeChangeP5(v, 1); > + sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE | = OPFLAG_EPH_INSERT); These flags are unlikely to be compatible: NCHANGE refers to changes occurred to db. Insertion to ephemeral space can=E2=80=99t change = anything in db. This snippet should look like (yep it initially contains bug: = ChangeP5(v, 1) should refer to OP_MakeRecord): +++ b/src/box/sql/update.c @@ -280,9 +280,10 @@ sqlite3Update(Parse * pParse, /* The = parser context */ = pPk->def); sqlite3VdbeAddOp4(v, OP_MakeRecord, iPk, pk_part_count, regKey, zAff, pk_part_count); + sqlite3VdbeChangeP5(v, 1); sqlite3VdbeAddOp2(v, OP_IdxInsert, regKey, reg_eph); /* Set flag to save memory allocating one by malloc. */ - sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE | = OPFLAG_EPH_INSERT); + sqlite3VdbeChangeP5(v, OPFLAG_EPH_INSERT); > } > /* End the database scan loop. > */ >=20 > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 00ffb0c..7df491b 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -3054,51 +3054,37 @@ case OP_TTransaction: { > break; > } >=20 > @@ -3107,66 +3093,60 @@ case OP_OpenWrite: > "need to re-compile SQL statement"); > goto abort_due_to_error; > } > - p2 =3D pOp->p2; > - struct space *space =3D pOp->p4.space; > + struct space *space; > + if (pOp->p4type =3D=3D P4_SPACEPTR) > + space =3D pOp->p4.space; > + else > + space =3D aMem[pOp->p3].u.p; > assert(space !=3D NULL); > - struct index *index =3D space_index(space, p2); > + struct index *index =3D space_index(space, pOp->p2); > assert(index !=3D NULL); > - /* > - * Since Tarantool iterator provides the full tuple, > - * we need a number of fields as wide as the table itself. > - * Otherwise, not enough slots for row parser cache are > - * allocated in VdbeCursor object. > - */ > - nField =3D space->def->field_count; > - assert(pOp->p1>=3D0); > - assert(nField>=3D0); > - pCur =3D allocateCursor(p, pOp->p1, nField, CURTYPE_TARANTOOL); > - if (pCur=3D=3D0) goto no_mem; > - pCur->nullRow =3D 1; > - pBtCur =3D pCur->uc.pCursor; > - pBtCur->curFlags |=3D BTCF_TaCursor; > - pBtCur->space =3D space; > - pBtCur->index =3D index; > - pBtCur->eState =3D CURSOR_INVALID; > + assert(pOp->p1 >=3D 0);=09 Trailing tab. > + cur =3D allocateCursor(p, pOp->p1, > + space->def->exact_field_count =3D=3D 0 ? > + space->def->field_count : > + space->def->exact_field_count, > + CURTYPE_TARANTOOL); > + if (cur =3D=3D NULL) > + goto no_mem; > + struct BtCursor *bt_cur =3D cur->uc.pCursor; > + bt_cur->curFlags |=3D space->def->id =3D=3D 0 ? = BTCF_TEphemCursor : > + BTCF_TaCursor; > + bt_cur->space =3D space; > + bt_cur->index =3D index; > + bt_cur->eState =3D CURSOR_INVALID; Grep CURSOR_INVALID - it is not used anywhere else. Mb it is worth to remove it? > /* Key info still contains sorter order and collation. */ > - pCur->key_def =3D index->def->key_def; > - > + cur->key_def =3D index->def->key_def; > + cur->nullRow =3D 1; > open_cursor_set_hints: > - pCur->uc.pCursor->hints =3D pOp->p5 & OPFLAG_SEEKEQ; > - if (rc) goto abort_due_to_error; > + cur->uc.pCursor->hints =3D pOp->p5 & OPFLAG_SEEKEQ; > + if (rc !=3D 0) > + goto abort_due_to_error; > break; > } >=20 > -/* Opcode: IdxInsert P1 P2 * * P5 > +/* Opcode: SorterInsert P1 P2 * * * > * Synopsis: key=3Dr[P2] > * > - * @param P1 Index of a space cursor. > - * @param P2 Index of a register with MessagePack data to insert. > + * Register P2 holds an SQL index key made using the > + * MakeRecord instructions. This opcode writes that key > + * into the sorter P1. Data for the entry is nil. > + */ > +case OP_SorterInsert: { /* in2 */ > + assert(pOp->p1 >=3D 0 && pOp->p1 < p->nCursor); > + struct VdbeCursor *cursor =3D p->apCsr[pOp->p1]; > + assert(cursor !=3D NULL); > + assert(isSorter(cursor)); > + pIn2 =3D &aMem[pOp->p2]; > + assert((pIn2->flags & MEM_Blob) !=3D 0); > + rc =3D ExpandBlob(pIn2); > + if (rc !=3D 0) > + goto abort_due_to_error; > + rc =3D sqlite3VdbeSorterWrite(cursor, pIn2); > + if (rc !=3D 0) > + goto abort_due_to_error; > + break; > +} > + > +/* Opcode: IdxInsert2 P1 * * P4 P5 P2 and P3 are used, so remove stars for clarity's sake. > + * Synopsis: key=3Dr[P1] > + * > + * @param P1 Index of a register with MessagePack data to insert. > + * @param P2 If P4 is not set, then P2 is register containing pointer > + * to space to insert into. > + * @param P4 Pointer to the struct space to insert to. > * @param P5 Flags. If P5 contains OPFLAG_NCHANGE, then VDBE > * accounts the change in a case of successful insertion in > * nChange counter. If P5 contains OPFLAG_OE_IGNORE, then > * we are processing INSERT OR INGORE statement. Thus, in > * case of conflict we don't raise an error. > */ > -/* Opcode: IdxReplace P1 P2 * * P5 > - * Synopsis: key=3Dr[P2] > +/* Opcode: IdxReplace2 P1 * * P4 P5 > + * Synopsis: key=3Dr[P1] > * > * This opcode works exactly as IdxInsert does, but in Tarantool > * internals it invokes box_replace() instead of box_insert(). > */ > -/* Opcode: SorterInsert P1 P2 * * * > - * Synopsis: key=3Dr[P2] > - * > - * Register P2 holds an SQL index key made using the > - * MakeRecord instructions. This opcode writes that key > - * into the sorter P1. Data for the entry is nil. > - */ > -case OP_SorterInsert: /* in2 */ > case OP_IdxReplace: > -case OP_IdxInsert: { /* in2 */ > - VdbeCursor *pC; > - > - assert(pOp->p1>=3D0 && pOp->p1nCursor); > - pC =3D p->apCsr[pOp->p1]; > - assert(pC!=3D0); > - assert(isSorter(pC)=3D=3D(pOp->opcode=3D=3DOP_SorterInsert)); > - pIn2 =3D &aMem[pOp->p2]; > - assert(pIn2->flags & MEM_Blob); > - if (pOp->p5 & OPFLAG_NCHANGE) p->nChange++; > - assert(pC->eCurType=3D=3DCURTYPE_TARANTOOL || = pOp->opcode=3D=3DOP_SorterInsert); > +case OP_IdxInsert: { > + pIn2 =3D &aMem[pOp->p1]; > + assert((pIn2->flags & MEM_Blob) !=3D 0); > + if (pOp->p5 & OPFLAG_NCHANGE) > + p->nChange++; Broken indentation. > rc =3D ExpandBlob(pIn2); > - if (rc) goto abort_due_to_error; > - if (pOp->opcode=3D=3DOP_SorterInsert) { > - rc =3D sqlite3VdbeSorterWrite(pC, pIn2); > + if (rc !=3D 0) > + goto abort_due_to_error; Broken indentation. > + struct space *space; > + if (pOp->p4type =3D=3D P4_SPACEPTR) { > + space =3D pOp->p4.space; > } else { > - BtCursor *pBtCur =3D pC->uc.pCursor; > - if (pBtCur->curFlags & BTCF_TaCursor) { > - /* Make sure that memory has been allocated on = region. */ > - assert(aMem[pOp->p2].flags & MEM_Ephem); > - if (pOp->opcode =3D=3D OP_IdxInsert) > - rc =3D = tarantoolSqlite3Insert(pBtCur->space, > - pIn2->z, > - pIn2->z + = pIn2->n); > - else > - rc =3D = tarantoolSqlite3Replace(pBtCur->space, > - pIn2->z, > - pIn2->z + = pIn2->n); > - } else if (pBtCur->curFlags & BTCF_TEphemCursor) { > - rc =3D = tarantoolSqlite3EphemeralInsert(pBtCur->space, > - pIn2->z, > - pIn2->z + = pIn2->n); > + space =3D aMem[pOp->p2].u.p; > + } > + assert(space !=3D NULL); > + if ((pOp->p5 & OPFLAG_EPH_INSERT) =3D=3D 0) { Why do you need this flag at all? You have space here and you can simply check its id to point out whether space is ephemeral or not. Throwing out this flag would allow you drop typical sqlite3VdbeChangeP5(v, OPFLAG_EPH_INSERT); usage which looks quite annoying. > + /* Make sure that memory has been allocated on region. = */ > + assert(aMem[pOp->p1].flags & MEM_Ephem); > + if (pOp->opcode =3D=3D OP_IdxInsert) { > + rc =3D tarantoolSqlite3Insert(space, > + pIn2->z, > + pIn2->z + pIn2->n); > } else { > - unreachable(); > + rc =3D tarantoolSqlite3Replace(space, > + pIn2->z, > + pIn2->z + pIn2->n); > } > - pC->cacheStatus =3D CACHE_STALE; > + } else { > + rc =3D tarantoolSqlite3EphemeralInsert(space, > + pIn2->z, > + pIn2->z + pIn2->n); > } >=20 > if (pOp->p5 & OPFLAG_OE_IGNORE) { > /* Ignore any kind of failes and do not raise error = message */ > rc =3D SQLITE_OK; > /* If we are in trigger, increment ignore raised counter = */ > - if (p->pFrame) { > + if (p->pFrame) > p->ignoreRaised++; > - } > } else if (pOp->p5 & OPFLAG_OE_FAIL) { > p->errorAction =3D ON_CONFLICT_ACTION_FAIL; > } else if (pOp->p5 & OPFLAG_OE_ROLLBACK) { > p->errorAction =3D ON_CONFLICT_ACTION_ROLLBACK; > } > - if (rc) goto abort_due_to_error; > + if (rc !=3D 0) > + goto abort_due_to_error; > break; > } I suggest cosmetic refactoring: +++ b/src/box/sql/vdbe.c @@ -4289,32 +4289,28 @@ case OP_IdxInsert: { pIn2 =3D &aMem[pOp->p1]; assert((pIn2->flags & MEM_Blob) !=3D 0); if (pOp->p5 & OPFLAG_NCHANGE) - p->nChange++; + p->nChange++; rc =3D ExpandBlob(pIn2); if (rc !=3D 0) - goto abort_due_to_error; + goto abort_due_to_error; struct space *space; - if (pOp->p4type =3D=3D P4_SPACEPTR) { + if (pOp->p4type =3D=3D P4_SPACEPTR) space =3D pOp->p4.space; - } else { + else space =3D aMem[pOp->p2].u.p; - } assert(space !=3D NULL); if ((pOp->p5 & OPFLAG_EPH_INSERT) =3D=3D 0) { /* Make sure that memory has been allocated on region. = */ assert(aMem[pOp->p1].flags & MEM_Ephem); if (pOp->opcode =3D=3D OP_IdxInsert) { - rc =3D tarantoolSqlite3Insert(space, - pIn2->z, + rc =3D tarantoolSqlite3Insert(space, pIn2->z, pIn2->z + pIn2->n); } else { - rc =3D tarantoolSqlite3Replace(space, - pIn2->z, + rc =3D tarantoolSqlite3Replace(space, pIn2->z, pIn2->z + pIn2->n); } } else { - rc =3D tarantoolSqlite3EphemeralInsert(space, - pIn2->z, + rc =3D tarantoolSqlite3EphemeralInsert(space, pIn2->z, pIn2->z + pIn2->n); } =20 @@ -4330,7 +4326,7 @@ case OP_IdxInsert: { p->errorAction =3D ON_CONFLICT_ACTION_ROLLBACK; } if (rc !=3D 0) - goto abort_due_to_error; + goto abort_due_to_error; break; } >=20 > @@ -4591,7 +4588,7 @@ sqlite3WhereBegin(Parse * pParse, /* The = parser context */ > if (pLoop->wsFlags & WHERE_INDEXED) { > struct index_def *idx_def =3D pLoop->index_def; > int iIndexCur; > - int op =3D OP_OpenRead; > + int op =3D OP_CursorOpen; > /* Check if index is primary. Either of > * points should be true: > * 1. struct Index is non-NULL and is > @@ -4636,12 +4633,12 @@ sqlite3WhereBegin(Parse * pParse, /* The = parser context */ > } > } > assert(wctrlFlags & = WHERE_ONEPASS_DESIRED); > - op =3D OP_OpenWrite; > + op =3D OP_CursorOpen; It is redundant assignment: you initialise this variable with = OP_CursorOpen value. > diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c > index e2aaca3..14278f0 100644 > --- a/src/box/sql/wherecode.c > +++ b/src/box/sql/wherecode.c > @@ -1341,7 +1341,8 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo, = /* Complete information about t > int iCovCur =3D pParse->nTab++; /* Cursor used for index = scans (if any) */ >=20 > int regReturn =3D ++pParse->nMem; /* Register used = with OP_Gosub */ > - int regRowset =3D 0; /* Register for RowSet object */ > + int regRowset =3D 0; /* Cursor for RowSet object */ regRowset now is really cursor, so lets better call like = =E2=80=98cursor_row_set=E2=80=99. Otherwise, it seems to be very confusing to operate on two similar variables: regRowset and reg_row_set... > + int reg_row_set =3D 0; > int regPk =3D 0; /* Register holding PK */ > int iLoopBody =3D sqlite3VdbeMakeLabel(v); /* Start = of loop body */ > int iRetInit; /* Address of regReturn init */ > @@ -1400,8 +1401,11 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * = pWInfo, /* Complete information about t > */ > if ((pWInfo->wctrlFlags & WHERE_DUPLICATES_OK) =3D=3D 0) = { > regRowset =3D pParse->nTab++; > + reg_row_set =3D ++pParse->nMem; > sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, > - regRowset, pk_part_count); > + reg_row_set, pk_part_count); > + sqlite3VdbeAddOp3(v, OP_CursorOpen, regRowset, = 0, > + reg_row_set); > sql_vdbe_set_p4_key_def(pParse, pk_key_def); > regPk =3D ++pParse->nMem; > }