Tarantool development patches archive
 help / color / mirror / Atom feed
From: "n.pettik" <korablev@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite
Date: Fri, 23 Mar 2018 19:01:18 +0300	[thread overview]
Message-ID: <E0B9DE6B-AAE9-41C5-ABD5-29103FE42218@tarantool.org> (raw)
In-Reply-To: <DE79EFDA-E63D-4E9C-B378-2CFC3BA7236C@tarantool.org>


> On 22 Mar 2018, at 14:04, v.shpilevoy@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.

  reply	other threads:[~2018-03-23 16:01 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-20 23:48 [tarantool-patches] [PATCH 0/4] Replace space id with space ptrs in VDBE runtime Nikita Pettik
2018-03-20 23:48 ` [tarantool-patches] [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite Nikita Pettik
2018-03-21 13:14   ` [tarantool-patches] " Kirill Yukhin
2018-03-22 10:07     ` n.pettik
2018-03-22 11:04       ` v.shpilevoy
2018-03-23 16:01         ` n.pettik [this message]
2018-03-20 23:48 ` [tarantool-patches] [PATCH 2/4] sql: introduce opcodes to operate on system spaces Nikita Pettik
2018-03-22 11:42   ` [tarantool-patches] " v.shpilevoy
2018-03-22 12:23     ` n.pettik
2018-03-22 13:09       ` v.shpilevoy
2018-03-23 16:20         ` n.pettik
2018-03-20 23:48 ` [tarantool-patches] [PATCH 3/4] sql: rework code generation for DDL routine Nikita Pettik
2018-03-22 13:57   ` [tarantool-patches] " v.shpilevoy
2018-03-23 16:33     ` n.pettik
2018-03-20 23:48 ` [tarantool-patches] [PATCH 4/4] sql: rework OP_OpenWrite/OpenRead Nikita Pettik
2018-03-22 14:11   ` [tarantool-patches] " v.shpilevoy
2018-03-23 16:39     ` n.pettik
2018-03-24 12:37 ` [tarantool-patches] Re: [PATCH 0/4] Replace space id with space ptrs in VDBE runtime v.shpilevoy
2018-03-27 16:28   ` n.pettik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=E0B9DE6B-AAE9-41C5-ABD5-29103FE42218@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='[tarantool-patches] Re: [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox