[tarantool-patches] Re: [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite
v.shpilevoy at tarantool.org
v.shpilevoy at tarantool.org
Thu Mar 22 14:04:00 MSK 2018
See below 1 comment, and 1 off topic comment.
And look at travis - it fails on the branch.
>
> Originally in SQLite, to open table (i.e. btree) it was required to pass
> number of page root to OP_OpenRead or OP_OpenWrite opcodes as an
> argument. However, now there are only Tarantool spaces and nothing
> prevents from operating directly on pointers to them. Thus, to pass
> pointers from compile time to runtime, opcode OP_LoadPtr has been
> introduced. It fetches pointer from P4 and stores to the register
> specified by P2.
> It is worth mentioning that, pointers are able to expire after schema
> changes (i.e. after DML routine). For this reason, schema version is
> saved to VDBE at compile time and checked each time during cursor
> opening.
>
> Part of #3252
> ---
> src/box/sql/analyze.c | 17 +++++-
> src/box/sql/build.c | 7 ++-
> src/box/sql/expr.c | 14 ++++-
> src/box/sql/fkey.c | 11 +++-
> src/box/sql/insert.c | 36 ++++++++++--
> src/box/sql/opcodes.c | 137 +++++++++++++++++++++----------------------
> src/box/sql/opcodes.h | 157 +++++++++++++++++++++++++-------------------------
> src/box/sql/select.c | 11 +++-
> src/box/sql/vdbe.c | 13 +++++
> src/box/sql/vdbe.h | 2 +
> src/box/sql/vdbeInt.h | 3 +
> src/box/sql/vdbeaux.c | 11 ++++
> src/box/sql/where.c | 12 +++-
> 13 files changed, 272 insertions(+), 159 deletions(-)
>
> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index db06d0182..d121dd2b9 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -174,10 +174,16 @@ openStatTable(Parse * pParse, /* Parsing context */
>
> /* Open the sql_stat[134] tables for writing. */
> for (i = 0; aTable[i]; i++) {
> + struct space *space =
> + space_by_id(SQLITE_PAGENO_TO_SPACEID(aRoot[i]));
> + assert(space != NULL);
> + int space_ptr_reg = ++pParse->nMem;
> + sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0,
> + (void *) space);
> int addr;
> addr =
> sqlite3VdbeAddOp3(v, OP_OpenWrite, iStatCur + i, aRoot[i],
> - 0);
> + space_ptr_reg);
1. Why do you pass here p3, if it is not used in OP_OpenWrite? Why can not you just use sqlite3VdbeAddOp2 here? Same about OpenRead in all the diff below. And how about to wrap this pair of calls sqlite3VdbeAddOp4Ptr(space) + sqlite3VdbeAddOp3(open_read/write) into a separate function? This code is repeated many times, as I can see.
> v->aOp[addr].p4.pKeyInfo = 0;
> v->aOp[addr].p4type = P4_KEYINFO;
> sqlite3VdbeChangeP5(v, aCreateTbl[i]);
> @@ -814,6 +820,7 @@ analyzeOneTable(Parse * pParse, /* Parser context */
> int iTabCur; /* Table cursor */
> Vdbe *v; /* The virtual machine being built up */
> int i; /* Loop counter */
> + int space_ptr_reg = iMem++;
Off topic: How about create a mempool of Mems? I see, that this object is created really often and in big amount (see src/lib/small/small/mempool.h). (It is not a part of the patch - just think about it, and possibly open a ticket, if it worth).
>
>
>
More information about the Tarantool-patches
mailing list