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

[n.pettik]

> On 22 Mar 2018, at 14:04, v.shpilevoy at tarantool.org wrote:
> 
> See below 1 comment, and 1 off topic comment.
> 
> And look at travis - it fails on the branch.

It was tricky bug which I didn’t manage to reproduce on macOS.
Travis failed on box.cfg{} when initialising SQL subsystem.
(Somehow I used sqlite3_free() on memory allocated by ordinary malloc.)

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

In this patch I don’t update OP_OpenWrite, since it will be still incomplete.
Only after introducing new DDL it will be possible to refactor OP_OpenWrite.

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

Done (updated on branch):

@@ -1160,6 +1160,30 @@ index_collation_name(Index *idx, uint32_t column)
        return index->def->key_def->parts[column].coll->name;
 }

+/**
+ * Create cursor which will be positioned to the space/index.
+ * It makes space lookup and loads pointer to it into register,
+ * which is passes to OP_OpenWrite as an argument.
+ *
+ * @param parse_context Parse context.
+ * @param cursor Number of cursor to be created.
+ * @param entity_id Encoded space and index ids.
+ * @retval address of last opcode.
+ */
+int
+emit_open_cursor(Parse *parse_context, int cursor, int entity_id)
+{
+       assert(entity_id > 0);
+       struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(entity_id));
+       assert(space != NULL);
+       Vdbe *vdbe = parse_context->pVdbe;
+       int space_ptr_reg = ++parse_context->nMem;
+       sqlite3VdbeAddOp4Ptr(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0,
+                            (void *) space);
+       return sqlite3VdbeAddOp3(vdbe, OP_OpenWrite, cursor, entity_id,
+                                space_ptr_reg);
+}
+


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

Memory for ’struct Mem’ objects is allocated at once (at sqlite3VdbeMakeReady()), so I don’t think that there will be any benefit from mempool.