[tarantool-patches] Re: [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite

["v.shpilevoy@tarantool.org" <v.shpilevoy@tarantool.org>]

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).
> 
> 
>