On 21 Mar 2018, at 16:14, Kirill Yukhin <kyukhin@tarantool.org> wrote:

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.