[tarantool-patches] Re: [PATCH 2/4] sql: introduce opcodes to operate on system spaces
v.shpilevoy at tarantool.org
v.shpilevoy at tarantool.org
Thu Mar 22 14:42:37 MSK 2018
See below 6 comments.
> 21 марта 2018 г., в 2:48, Nikita Pettik <korablev at 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;
> +}
> +
>
>
More information about the Tarantool-patches
mailing list