From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 34D8D29314 for ; Wed, 21 Mar 2018 09:14:54 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id m81qZYxswHUH for ; Wed, 21 Mar 2018 09:14:54 -0400 (EDT) Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id E8AAA267DC for ; Wed, 21 Mar 2018 09:14:53 -0400 (EDT) Date: Wed, 21 Mar 2018 16:14:50 +0300 From: Kirill Yukhin Subject: [tarantool-patches] Re: [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite Message-ID: <20180321131450.otxkluudlhf3yh2i@tarantool.org> References: <46d0750257b6b5a256210b063515b1b05f4d7d37.1521583434.git.korablev@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <46d0750257b6b5a256210b063515b1b05f4d7d37.1521583434.git.korablev@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Nikita Pettik Hello, My comments inlined. On 21 мар 02:48, Nikita Pettik wrote: > 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. On the other hand, > 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 | 37 ++++++++++++++++++++++++++++++++----- > src/box/sql/select.c | 11 ++++++++++- > src/box/sql/vdbe.c | 12 ++++++++++-- > src/box/sql/vdbe.h | 1 + > src/box/sql/vdbeInt.h | 1 + > src/box/sql/vdbeaux.c | 13 +++++++++++++ > src/box/sql/where.c | 12 +++++++++++- > 11 files changed, 121 insertions(+), 15 deletions(-) > > diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c > index db06d0182..57fc33c70 100644 > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 9929dfb96..5d1227afa 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -3217,9 +3217,17 @@ case OP_OpenWrite: > > assert(p2 >= 1); > pBtCur = pCur->uc.pCursor; > - pBtCur->space = space_by_id(SQLITE_PAGENO_TO_SPACEID(p2)); > + if (box_schema_version() == p->schema_ver) { > + pIn3 = &aMem[pOp->p3]; > + /* Make sure that 64-bit pointer can fit into int64_t. */ > + assert(sizeof(pBtCur->space) >= sizeof(pIn3->u.i)); I don't like this. If we're going to extensively use pointers space/index then let's extend Memory struct adding dedicated types to the union. Note, that new opcode (say, LoadPtr) will be needed. > + pBtCur->space = ((struct space *) pIn3->u.i); > + } else { > + pBtCur->space = space_by_id(SQLITE_PAGENO_TO_SPACEID(p2)); > + } Don't surround single stmt withcurly braces pls. Also, if schema was changed then error should be returned (stmt expired or smth). > diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h > index 7241963e4..a1ecf729d 100644 > --- a/src/box/sql/vdbe.h > +++ b/src/box/sql/vdbe.h > diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h > index 8b622de5b..99262ab7b 100644 > --- a/src/box/sql/vdbeInt.h > +++ b/src/box/sql/vdbeInt.h > @@ -378,6 +378,7 @@ struct Vdbe { > i64 nFkConstraint; /* Number of imm. FK constraints this VM */ > i64 nStmtDefCons; /* Number of def. constraints when stmt started */ > i64 nStmtDefImmCons; /* Number of def. imm constraints when stmt started */ > + uint32_t schema_ver; /* Schema version at the moment of VDBE creation. */ > > /* > * In recursive triggers we can execute INSERT/UPDATE OR IGNORE > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c > index 92bf9943b..b35d0712e 100644 > --- a/src/box/sql/vdbeaux.c > +++ b/src/box/sql/vdbeaux.c > @@ -66,6 +66,7 @@ sqlite3VdbeCreate(Parse * pParse) > p->magic = VDBE_MAGIC_INIT; > p->pParse = pParse; > p->autoCommit = (char)box_txn() == 0 ? 1 : 0; > + p->schema_ver = box_schema_version(); > if (!p->autoCommit) { > p->psql_txn = in_txn()->psql_txn; > p->nDeferredCons = p->psql_txn->nDeferredConsSave; > @@ -413,6 +414,18 @@ sqlite3VdbeAddOp4Int(Vdbe * p, /* Add the opcode to this VM */ > return addr; > } > > +int > +sqlite3VdbeAddOp4Int64(Vdbe *p, int op, int p1, int p2, int p3, int64_t p4) > +{ > + int addr = sqlite3VdbeAddOp3(p, op, p1, p2, p3); > + VdbeOp *pOp = &p->aOp[addr]; > + pOp->p4type = P4_INT64; > + pOp->p4.pI64 = sqlite3DbMallocRawNN(sqlite3VdbeDb(p), sizeof(int64_t)); > + if (p->db->mallocFailed == 0) > + *pOp->p4.pI64 = p4; > + return addr; > +} This is useless if LoadPTR will be introduced. -- Thanks, Kirill