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

[Kirill Yukhin <kyukhin@tarantool.org>]

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