[tarantool-patches] Re: [PATCH 2/4] sql: introduce opcodes to operate on system spaces

["v.shpilevoy@tarantool.org" <v.shpilevoy@tarantool.org>]

See below 6 comments.

> 21 марта 2018 г., в 2:48, Nikita Pettik <korablev@tarantool.org> написал(а):
> 
> Since it is impossible to use space pointers during execution of DDL
> (after any DDL schema may change and pointers fetched at compile time
> can become expired), special opcodes to operate on system spaces have
> been introduced: OP_SInsert and OP_SDelete. They take space id as
> an argument and make space lookup each time before insertion or deletion.
> However, sometimes it is still required to iterate through space (e.g.
> to satisfy WHERE clause) during DDL. As far as now cursors rely on
> pointers to space, it is required to convert space id to space pointer
> during VDBE execution. Hence, another opcode is added: OP_SIDtoPtr.
> Finally, existing opcodes, which are invoked only during DDL, have been
> also refactored.
> 
> Part of #3252
> ---
> src/box/sql.c              |  51 ++++++++++++-------
> src/box/sql/opcodes.c      |  45 +++++++++--------
> src/box/sql/opcodes.h      |  53 ++++++++++----------
> src/box/sql/tarantoolInt.h |  11 ++---
> src/box/sql/vdbe.c         | 121 ++++++++++++++++++++++++++++++++++-----------
> 5 files changed, 182 insertions(+), 99 deletions(-)
> 
> diff --git a/src/box/sql.c b/src/box/sql.c
> index e943131ae..db4c7e7ea 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> +int
> +sql_delete_by_key(struct space *space, char *key, uint32_t key_size)
> +{
> 	struct request request;
> 	memset(&request, 0, sizeof(request));
> 	request.type = IPROTO_DELETE;
> 	request.key = key;
> 	request.key_end = key + key_size;
> -	request.space_id = pCur->space->def->id;
> -	rc = sql_execute_dml(&request, pCur->space);
> +	request.space_id = space->def->id;
> +	int rc = sql_execute_dml(&request, space);

1. Why do you need sql_execute_dml? It does exactly the same as process_rw, but process_rw can also return a tuple. And this process_rw feature is needed for the patch on last inserted tuple returning.
> 
> @@ -1088,20 +1107,16 @@ int tarantoolSqlite3IncrementMaxid(BtCursor *pCur)
> 	request.ops = ops;
> 	request.ops_end = ops + sizeof(ops);
> 	request.type = IPROTO_UPSERT;
> -	request.space_id = pCur->space->def->id;
> -	rc = sql_execute_dml(&request, pCur->space);
> +	request.space_id = BOX_SCHEMA_ID;
> +	rc = sql_execute_dml(&request, space_schema);

2. If you use process_rw, then you do not need iterator to get the previous max_id and increment it - you just do process_rw, get result and do tuple_field_u64(result, 1, space_max_id);

> 	if (rc != 0) {
> 		iterator_delete(it);
> 		return SQL_TARANTOOL_ERROR;
> 	}
> -	if (pCur->last_tuple != NULL) {
> -		box_tuple_unref(pCur->last_tuple);
> -	}
> -	box_tuple_ref(res);
> -	pCur->last_tuple = res;
> -	pCur->eState = CURSOR_VALID;
> +	rc = tuple_field_u64(res, 1, space_max_id);
> +	(*space_max_id)++;
> 	iterator_delete(it);
> -	return SQLITE_OK;
> +	return rc  == 0 ? SQLITE_OK : SQL_TARANTOOL_ERROR;
> }
> 
> /*
> @@ -1687,14 +1702,12 @@ int tarantoolSqlite3EphemeralGetMaxId(BtCursor *pCur, uint32_t fieldno,
>  * If index is empty - return 0 in max_id and success status
>  */
> int
> -tarantoolSqlGetMaxId(BtCursor *cur, uint32_t fieldno,
> -		     uint64_t *max_id)
> +tarantoolSqlGetMaxId(uint32_t space_id, uint64_t *max_id)

3. After changing a function signature, please, fix a comments too. And why this function is sure, that the max id stored in the first column? And why it can not be used for secondary indexes?

> {
> 	char key[16];
> 	struct tuple *tuple;
> 	char *key_end = mp_encode_array(key, 0);
> -	if (box_index_max(cur->space->def->id, cur->index->def->iid,
> -			  key, key_end, &tuple) != 0)
> +	if (box_index_max(space_id, 0 /* PK */, key, key_end, &tuple) != 0)
> 		return -1;
> 
> 	/* Index is empty  */
> @@ -1703,5 +1716,5 @@ tarantoolSqlGetMaxId(BtCursor *cur, uint32_t fieldno,
> 		return 0;
> 	}
> 
> -	return tuple_field_u64(tuple, fieldno, max_id);
> +	return tuple_field_u64(tuple, 0, max_id);
> }
> diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
> index 6f1ba3784..0b1e22ca2 100644
> --- a/src/box/sql/tarantoolInt.h
> +++ b/src/box/sql/tarantoolInt.h
> @@ -77,6 +77,7 @@ int tarantoolSqlite3Count(BtCursor * pCur, i64 * pnEntry);
> int tarantoolSqlite3Insert(BtCursor * pCur);
> int tarantoolSqlite3Replace(BtCursor * pCur);
> int tarantoolSqlite3Delete(BtCursor * pCur, u8 flags);
> +int sql_delete_by_key(struct space *space, char *key, uint32_t key_size);
> int tarantoolSqlite3ClearTable(struct space *space);
> 
> /* Rename table pTab with zNewName by inserting new tuple to _space.
> @@ -112,11 +113,10 @@ int tarantoolSqlite3IdxKeyCompare(BtCursor * pCur, UnpackedRecord * pUnpacked,
> 				  int *res);
> 
> /*
> - * The function assumes the cursor is open on _schema.
> - * Increment max_id and store updated tuple in the cursor
> - * object.
> + * The function assumes to be applied on _schema space.
> + * Increment max_id and store updated id in given argument.
>  */
> -int tarantoolSqlite3IncrementMaxid(BtCursor * pCur);
> +int tarantoolSqlite3IncrementMaxid(uint64_t *space_max_id);
> 
> /*
>  * Render "format" array for _space entry.
> @@ -150,5 +150,4 @@ int tarantoolSqlite3MakeIdxOpts(Index * index, const char *zSql, void *buf);
>  * Fetch maximum value from ineger column number `fieldno` of space_id/index_id
>  * Return 0 on success, -1 otherwise
>  */
> -int tarantoolSqlGetMaxId(BtCursor *cur, uint32_t fieldno,
> -			 uint64_t * max_id);
> +int tarantoolSqlGetMaxId(uint32_t space_id, uint64_t *max_id);
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 5d1227afa..d333d4177 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -3791,28 +3791,18 @@ case OP_Sequence: {           /* out2 */
> 	break;
> }
> 
> -/* Opcode: NextId P1 P2 P3 * *
> - * Synopsis: r[P3]=get_max(space_index[P1]{Column[P2]})
> +/* Opcode: NextId P1 P2 * * *
> + * Synopsis: r[P2]=get_max(space_id[P1])
>  *
> - * Get next Id of the table. P1 is a table cursor, P2 is column
> - * number. Return in P3 maximum id found in provided column,
> + * Get next Id of the table. P1 is a space id.
> + * Return in P2 maximum id found in provided column,
>  * incremented by one.
> - *
> - * This opcode is Tarantool specific and will segfault in case
> - * of SQLite cursor.
>  */
> -case OP_NextId: {     /* out3 */
> -	VdbeCursor *pC;    /* The VDBE cursor */
> -	int p2;            /* Column number, which stores the id */
> -	pC = p->apCsr[pOp->p1];
> -	p2 = pOp->p2;
> -	pOut = &aMem[pOp->p3];
> -
> -	/* This opcode is Tarantool specific.  */
> -	assert(pC->uc.pCursor->curFlags & BTCF_TaCursor);
> +case OP_NextId: {
> +	assert(pOp->p1 > 0);
> +	pOut = &aMem[pOp->p2];
> 
> -	tarantoolSqlGetMaxId(pC->uc.pCursor, p2,
> -			     (uint64_t *) &pOut->u.i);
> +	tarantoolSqlGetMaxId(pOp->p1, (uint64_t *) &pOut->u.i);
> 
> 	pOut->u.i += 1; 
> 	pOut->flags = MEM_Int;
> @@ -4456,6 +4446,84 @@ case OP_IdxInsert: {        /* in2 */
> 	break;
> }
> 
> +/* Opcode: SInsert P1 P2 * * P5
> + * Synopsis: space id = P1, key = r[P2]
> + *
> + * This opcode is used only during DML routine.

4. Do you mean DDL instead of DML?

> + * In contrast to ordinary insertion, insertion to system spaces
> + * such as _space or _index will lead to schema changes.
> + * Thus, usage of space pointers is going to be impossible,
> + * as far as pointers can be expired since compilation time.
> + *
> + * If P5 is set to OPFLAG_NCHANGE, account overall changes
> + * made to database.
> + */
> +case OP_SInsert: {
> +	assert(pOp->p1 > 0);
> +	assert(pOp->p2 >= 0);
> +
> +	pIn2 = &aMem[pOp->p2];
> +	struct space *space = space_by_id(pOp->p1);
> +	assert(space != NULL);
> +	assert(space_is_system(space));
> +	/* Create surrogate cursor to pass to SQL bindings. */
> +	BtCursor surrogate_cur;
> +	surrogate_cur.space = space;
> +	surrogate_cur.key = pIn2->z;
> +	surrogate_cur.nKey = pIn2->n;
> +	surrogate_cur.curFlags = BTCF_TaCursor;
> +	rc = tarantoolSqlite3Insert(&surrogate_cur);
> +	if (rc)
> +		goto abort_due_to_error;
> +	if (pOp->p5 & OPFLAG_NCHANGE)
> +		p->nChange++;
> +	break;
> +}
> +
> +/* Opcode: SDelete P1 P2 * * P5
> + * Synopsis: space id = P1, key = r[P2]
> + *
> + * This opcode is used only during DML routine.

5. Same as 4 - do you mean DDL?

> + * Delete entry with given key from system space.
> + *
> + * If P5 is set to OPFLAG_NCHANGE, account overall changes
> + * made to database.
> + */
> +case OP_SDelete: {
> +	assert(pOp->p1 > 0);
> +	assert(pOp->p2 >= 0);
> +
> +	pIn2 = &aMem[pOp->p2];
> +	struct space *space = space_by_id(pOp->p1);
> +	assert(space != NULL);
> +	assert(space_is_system(space));
> +	rc = sql_delete_by_key(space, pIn2->z, pIn2->n);
> +	if (rc)
> +		goto abort_due_to_error;
> +	if (pOp->p5 & OPFLAG_NCHANGE)
> +		p->nChange++;
> +	break;
> +}
> +
> +/* Opcode: SIDtoPtr P1 P2 * * *
> + * Synopsis: space id = P1, space[out] = r[P2]
> + *
> + * This opcode makes look up by space id and save found space
> + * into register, specified by the content of register P2.
> + * Such trick is needed during DML routine, since schema may
> + * change and pointers become expired.
> + */
> +case OP_SIDtoPtr: {

6. It seems to be unused in the patch. Can you use it for all these things, introduced by the previous patch?
struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
assert(space != NULL);
sqlite3VdbeAddOp4Ptr(v, OP_LoadPtr, 0, space_ptr_reg, 0, (void *) space);
sqlite3VdbeAddOp3(v, OP_OpenRead, iIdxCur, pIdx->tnum, space_ptr_reg);

Using OP_SIDtoPtr you already in this patch can defer space_by_id lookup.

> +	assert(pOp->p1 > 0);
> +	assert(pOp->p2 > 0);
> +
> +	pIn2 = &aMem[pOp->p2];
> +	struct space *space = space_by_id(pOp->p1);
> +	assert(space != NULL);
> +	pIn2->u.i = (int64_t) space;
> +	break;
> +}
> +
> 
>