[tarantool-patches] Re: [PATCH 1/4] sql: pass space pointer to OP_OpenRead/OpenWrite
Kirill Yukhin
kyukhin at tarantool.org
Wed Mar 21 16:14:50 MSK 2018
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
More information about the Tarantool-patches
mailing list