[tarantool-patches] Re: [PATCH] sql: refactor SQL cursor to remove write ones
n.pettik
korablev at tarantool.org
Tue Oct 2 14:56:54 MSK 2018
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 ‘pre-loaded’. 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 ‘cursor’ is used only for read iterations over the space.
> On 1 Oct 2018, at 13:31, Kirill Yukhin <kyukhin at tarantool.org> wrote:
>
> 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;
> }
>
> -/**
> - * Create ephemeral space and set cursor to the first entry. Features of
> - * ephemeral spaces: id == 0, name == "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);
> }
>
> 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 != NULL);
> - return sqlite3VdbeAddOp4(parse_context->pVdbe, OP_OpenWrite, cursor,
> + return sqlite3VdbeAddOp4(parse_context->pVdbe, OP_CursorOpen, cursor,
> index_id, 0, (void *) space, P4_SPACEPTR);
> }
>
> @@ -2693,7 +2693,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
> goto exit_create_index;
>
> 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)
>
> 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 = parse->db;
> where = sqlite3ExprDup(db, where, 0);
> @@ -83,7 +83,7 @@ sql_materialize_view(struct Parse *parse, const char *name, struct Expr *where,
> struct Select *select = 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 = ++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);
> }
>
> /* Initialize the counter of the number of rows deleted,
> @@ -390,6 +393,8 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>
> VdbeCoverage(v);
> } else {
> + sqlite3VdbeAddOp3(v, OP_CursorOpen,
> + eph_cursor, 0, reg_eph);
Broken indentation.
> addr_loop = 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 = pParse->nTab++;
> pExpr->is_ephemeral = 1;
> + int reg_eph = ++pParse->nMem;
> addr = 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 = sql_key_info_new(pParse->db, nVal);
> if (key_info == 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 =
> exprINAffinity(pParse, pExpr);
> assert((pExpr->iTable & 0x0000FFFF) ==
> @@ -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 */
>
> pSel = pExpr->x.pSelect;
> nReg = pExpr->op == TK_SELECT ? pSel->pEList->nExpr : 1;
> - sqlite3SelectDestInit(&dest, 0, pParse->nMem + 1);
> + sqlite3SelectDestInit(&dest, 0, pParse->nMem + 1, 0);
If you don’t 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.
>
> @@ -1327,7 +1334,8 @@ xferOptimization(Parse * pParse, /* Parser context */
> sqlite3VdbeChangeP5(v, OPFLAG_XFER_OPT);
> #endif
>
> - 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 ::= DROP VIEW ifexists(E) fullname(X). {
> //////////////////////// The SELECT statement /////////////////////////////////
> //
> cmd ::= select(X). {
> - SelectDest dest = {SRT_Output, 0, 0, 0, 0, 0};
> + SelectDest dest = {SRT_Output, 0, 0, 0, 0, 0, 0};
Nit: don’t 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 = parse->pVdbe;
> int r1;
>
> - v = pParse->pVdbe;
> - r1 = sqlite3GetTempReg(pParse);
> - sqlite3VdbeAddOp4Int(v, OP_Found, iTab, addrRepeat, iMem, N);
> + r1 = 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 */
}
/**
- * 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 = parse->pVdbe;
- int r1;
-
- r1 = sqlite3GetTempReg(parse);
+ int r1 = 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 */
}
default:{
- assert(pDistinct->eTnctType ==
- WHERE_DISTINCT_UNORDERED);
- codeDistinct(pParse, pDistinct->tabTnct, pDistinct->reg_eph,
- iContinue, nResultCol, regResult);
- break;
- }
+ assert(pDistinct->eTnctType ==
+ WHERE_DISTINCT_UNORDERED);
+ vdbe_insert_distinct(pParse, pDistinct->tabTnct,
+ pDistinct->reg_eph, iContinue,
+ nResultCol, regResult);
+ break;
+ }
}
if (pSort == 0) {
codeOffset(v, p->iOffset, iContinue);
@@ -5391,8 +5386,8 @@ updateAccumulator(Parse * pParse, AggInfo * pAggInfo)
addrNext = sqlite3VdbeMakeLabel(v);
testcase(nArg == 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);
>
> /*
> @@ -998,7 +1013,8 @@ selectInnerLoop(Parse * pParse, /* The parser context */
> * row is all NULLs.
> */
> sqlite3VdbeChangeToNoop(v, pDistinct->addrTnct);
> - pOp = sqlite3VdbeGetOp(v, pDistinct->addrTnct);
> + sqlite3VdbeChangeToNoop(v, pDistinct->addrTnct + 1);
> + pOp = sqlite3VdbeGetOp(v, pDistinct->addrTnct + 1);
Actually, I don’t 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’t dive into code).
> pOp->opcode = OP_Null;
> pOp->p1 = 1;
> pOp->p2 = regPrev;
> @@ -1040,13 +1056,14 @@ selectInnerLoop(Parse * pParse, /* The parser context */
>
> case WHERE_DISTINCT_UNIQUE:{
> sqlite3VdbeChangeToNoop(v, pDistinct->addrTnct);
> + sqlite3VdbeChangeToNoop(v, pDistinct->addrTnct + 1);
The same is here. Also, out of 80.
>
> @@ -3417,8 +3459,8 @@ multiSelectOrderBy(Parse * pParse, /* Parsing context */
> regAddrB = ++pParse->nMem;
> regOutA = ++pParse->nMem;
> regOutB = ++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.
>
> /* 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 == pAggInfo->mxReg - pAggInfo->mnReg + 1);
> + assert(nReg <= pAggInfo->mxReg - pAggInfo->mnReg + 1);
Hm? Explain pls this change.
> for (i = 0; i < pAggInfo->nColumn; i++) {
> assert(pAggInfo->aCol[i].iMem >= pAggInfo->mnReg
> && pAggInfo->aCol[i].iMem <= pAggInfo->mxReg);
>
> @@ -5840,13 +5891,16 @@ sqlite3Select(Parse * pParse, /* The parser context */
> */
> if (p->selFlags & SF_Distinct) {
> sDistinct.tabTnct = pParse->nTab++;
> + sDistinct.reg_eph = ++pParse->nMem;
> struct sql_key_info *key_info =
> sql_expr_list_to_key_info(pParse, p->pEList, 0);
> sDistinct.addrTnct = 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 = WHERE_DISTINCT_UNORDERED;
> } else {
> @@ -5886,6 +5940,7 @@ sqlite3Select(Parse * pParse, /* The parser context */
> */
> if (sSort.addrSortIndex >= 0 && sSort.pOrderBy == 0) {
> sqlite3VdbeChangeToNoop(v, sSort.addrSortIndex);
> + sqlite3VdbeChangeToNoop(v, sSort.addrSortIndex + 1);
Explain pls why do you need this.
> }
>
> /* Use the standard inner loop. */
> @@ -6130,6 +6185,7 @@ sqlite3Select(Parse * pParse, /* The parser context */
> ) {
> sSort.pOrderBy = 0;
> sqlite3VdbeChangeToNoop(v, sSort.addrSortIndex);
> + sqlite3VdbeChangeToNoop(v, sSort.addrSortIndex + 1);
The same.
> }
>
> /* 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!=0, 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<<N) set if column N of pTab is used */
> @@ -2591,6 +2595,8 @@ struct SelectDest {
> u8 eDest; /* How to dispose of the results. On of SRT_* above. */
> char *zAffSdst; /* Affinity used when eDest==SRT_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 */
>
> /**
> 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 =
> 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 = ++pParse->nMem;
> + sql_materialize_view(pParse, def->name, pWhere, pk_cursor, reg_eph);
> /* Number of columns from SELECT plus ID.*/
> pk_part_count = nKey = def->field_count + 1;
> } else {
> - vdbe_emit_open_cursor(pParse, pk_cursor, 0,
> - space_by_id(pTab->def->id));
> + space = space_by_id(pTab->def->id);
You don’t need to fetch space from cache: now real space is
stored in pTab->space (see sql_lookup_table()).
> + assert(space != NULL);
> + vdbe_emit_open_cursor(pParse, pk_cursor, 0, space);
> pk_part_count = pPk->def->key_def->part_count;
> }
>
> @@ -242,11 +245,12 @@ sqlite3Update(Parse * pParse, /* The parser context */
> int iPk = pParse->nMem + 1;
> pParse->nMem += pk_part_count;
> regKey = ++pParse->nMem;
> + int reg_eph = ++pParse->nMem;
> iEph = pParse->nTab++;
> sqlite3VdbeAddOp2(v, OP_Null, 0, iPk);
>
> /* Address of the OpenEphemeral instruction. */
> - int addrOpen = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, iEph,
> + int addrOpen = sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
> pk_part_count);
> pWInfo = 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’t 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.
> */
>
> 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;
> }
>
> @@ -3107,66 +3093,60 @@ case OP_OpenWrite:
> "need to re-compile SQL statement");
> goto abort_due_to_error;
> }
> - p2 = pOp->p2;
> - struct space *space = pOp->p4.space;
> + struct space *space;
> + if (pOp->p4type == P4_SPACEPTR)
> + space = pOp->p4.space;
> + else
> + space = aMem[pOp->p3].u.p;
> assert(space != NULL);
> - struct index *index = space_index(space, p2);
> + struct index *index = space_index(space, pOp->p2);
> assert(index != 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 = space->def->field_count;
> - assert(pOp->p1>=0);
> - assert(nField>=0);
> - pCur = allocateCursor(p, pOp->p1, nField, CURTYPE_TARANTOOL);
> - if (pCur==0) goto no_mem;
> - pCur->nullRow = 1;
> - pBtCur = pCur->uc.pCursor;
> - pBtCur->curFlags |= BTCF_TaCursor;
> - pBtCur->space = space;
> - pBtCur->index = index;
> - pBtCur->eState = CURSOR_INVALID;
> + assert(pOp->p1 >= 0);
Trailing tab.
> + cur = allocateCursor(p, pOp->p1,
> + space->def->exact_field_count == 0 ?
> + space->def->field_count :
> + space->def->exact_field_count,
> + CURTYPE_TARANTOOL);
> + if (cur == NULL)
> + goto no_mem;
> + struct BtCursor *bt_cur = cur->uc.pCursor;
> + bt_cur->curFlags |= space->def->id == 0 ? BTCF_TEphemCursor :
> + BTCF_TaCursor;
> + bt_cur->space = space;
> + bt_cur->index = index;
> + bt_cur->eState = 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 = index->def->key_def;
> -
> + cur->key_def = index->def->key_def;
> + cur->nullRow = 1;
> open_cursor_set_hints:
> - pCur->uc.pCursor->hints = pOp->p5 & OPFLAG_SEEKEQ;
> - if (rc) goto abort_due_to_error;
> + cur->uc.pCursor->hints = pOp->p5 & OPFLAG_SEEKEQ;
> + if (rc != 0)
> + goto abort_due_to_error;
> break;
> }
>
> -/* Opcode: IdxInsert P1 P2 * * P5
> +/* Opcode: SorterInsert P1 P2 * * *
> * Synopsis: key=r[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 >= 0 && pOp->p1 < p->nCursor);
> + struct VdbeCursor *cursor = p->apCsr[pOp->p1];
> + assert(cursor != NULL);
> + assert(isSorter(cursor));
> + pIn2 = &aMem[pOp->p2];
> + assert((pIn2->flags & MEM_Blob) != 0);
> + rc = ExpandBlob(pIn2);
> + if (rc != 0)
> + goto abort_due_to_error;
> + rc = sqlite3VdbeSorterWrite(cursor, pIn2);
> + if (rc != 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=r[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=r[P2]
> +/* Opcode: IdxReplace2 P1 * * P4 P5
> + * Synopsis: key=r[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=r[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>=0 && pOp->p1<p->nCursor);
> - pC = p->apCsr[pOp->p1];
> - assert(pC!=0);
> - assert(isSorter(pC)==(pOp->opcode==OP_SorterInsert));
> - pIn2 = &aMem[pOp->p2];
> - assert(pIn2->flags & MEM_Blob);
> - if (pOp->p5 & OPFLAG_NCHANGE) p->nChange++;
> - assert(pC->eCurType==CURTYPE_TARANTOOL || pOp->opcode==OP_SorterInsert);
> +case OP_IdxInsert: {
> + pIn2 = &aMem[pOp->p1];
> + assert((pIn2->flags & MEM_Blob) != 0);
> + if (pOp->p5 & OPFLAG_NCHANGE)
> + p->nChange++;
Broken indentation.
> rc = ExpandBlob(pIn2);
> - if (rc) goto abort_due_to_error;
> - if (pOp->opcode==OP_SorterInsert) {
> - rc = sqlite3VdbeSorterWrite(pC, pIn2);
> + if (rc != 0)
> + goto abort_due_to_error;
Broken indentation.
> + struct space *space;
> + if (pOp->p4type == P4_SPACEPTR) {
> + space = pOp->p4.space;
> } else {
> - BtCursor *pBtCur = 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 == OP_IdxInsert)
> - rc = tarantoolSqlite3Insert(pBtCur->space,
> - pIn2->z,
> - pIn2->z + pIn2->n);
> - else
> - rc = tarantoolSqlite3Replace(pBtCur->space,
> - pIn2->z,
> - pIn2->z + pIn2->n);
> - } else if (pBtCur->curFlags & BTCF_TEphemCursor) {
> - rc = tarantoolSqlite3EphemeralInsert(pBtCur->space,
> - pIn2->z,
> - pIn2->z + pIn2->n);
> + space = aMem[pOp->p2].u.p;
> + }
> + assert(space != NULL);
> + if ((pOp->p5 & OPFLAG_EPH_INSERT) == 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 == OP_IdxInsert) {
> + rc = tarantoolSqlite3Insert(space,
> + pIn2->z,
> + pIn2->z + pIn2->n);
> } else {
> - unreachable();
> + rc = tarantoolSqlite3Replace(space,
> + pIn2->z,
> + pIn2->z + pIn2->n);
> }
> - pC->cacheStatus = CACHE_STALE;
> + } else {
> + rc = tarantoolSqlite3EphemeralInsert(space,
> + pIn2->z,
> + pIn2->z + pIn2->n);
> }
>
> if (pOp->p5 & OPFLAG_OE_IGNORE) {
> /* Ignore any kind of failes and do not raise error message */
> rc = 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 = ON_CONFLICT_ACTION_FAIL;
> } else if (pOp->p5 & OPFLAG_OE_ROLLBACK) {
> p->errorAction = ON_CONFLICT_ACTION_ROLLBACK;
> }
> - if (rc) goto abort_due_to_error;
> + if (rc != 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 = &aMem[pOp->p1];
assert((pIn2->flags & MEM_Blob) != 0);
if (pOp->p5 & OPFLAG_NCHANGE)
- p->nChange++;
+ p->nChange++;
rc = ExpandBlob(pIn2);
if (rc != 0)
- goto abort_due_to_error;
+ goto abort_due_to_error;
struct space *space;
- if (pOp->p4type == P4_SPACEPTR) {
+ if (pOp->p4type == P4_SPACEPTR)
space = pOp->p4.space;
- } else {
+ else
space = aMem[pOp->p2].u.p;
- }
assert(space != NULL);
if ((pOp->p5 & OPFLAG_EPH_INSERT) == 0) {
/* Make sure that memory has been allocated on region. */
assert(aMem[pOp->p1].flags & MEM_Ephem);
if (pOp->opcode == OP_IdxInsert) {
- rc = tarantoolSqlite3Insert(space,
- pIn2->z,
+ rc = tarantoolSqlite3Insert(space, pIn2->z,
pIn2->z + pIn2->n);
} else {
- rc = tarantoolSqlite3Replace(space,
- pIn2->z,
+ rc = tarantoolSqlite3Replace(space, pIn2->z,
pIn2->z + pIn2->n);
}
} else {
- rc = tarantoolSqlite3EphemeralInsert(space,
- pIn2->z,
+ rc = tarantoolSqlite3EphemeralInsert(space, pIn2->z,
pIn2->z + pIn2->n);
}
@@ -4330,7 +4326,7 @@ case OP_IdxInsert: {
p->errorAction = ON_CONFLICT_ACTION_ROLLBACK;
}
if (rc != 0)
- goto abort_due_to_error;
+ goto abort_due_to_error;
break;
}
>
> @@ -4591,7 +4588,7 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */
> if (pLoop->wsFlags & WHERE_INDEXED) {
> struct index_def *idx_def = pLoop->index_def;
> int iIndexCur;
> - int op = OP_OpenRead;
> + int op = 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 = OP_OpenWrite;
> + op = 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 = pParse->nTab++; /* Cursor used for index scans (if any) */
>
> int regReturn = ++pParse->nMem; /* Register used with OP_Gosub */
> - int regRowset = 0; /* Register for RowSet object */
> + int regRowset = 0; /* Cursor for RowSet object */
regRowset now is really cursor, so lets better call like ‘cursor_row_set’.
Otherwise, it seems to be very confusing to operate on two
similar variables: regRowset and reg_row_set...
> + int reg_row_set = 0;
> int regPk = 0; /* Register holding PK */
> int iLoopBody = 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) == 0) {
> regRowset = pParse->nTab++;
> + reg_row_set = ++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 = ++pParse->nMem;
> }
More information about the Tarantool-patches
mailing list