[tarantool-patches] Re: [PATCH 2/4] sql: introduce opcodes to operate on system spaces
n.pettik
korablev at tarantool.org
Thu Mar 22 15:23:51 MSK 2018
> On 22 Mar 2018, at 14:42, v.shpilevoy at tarantool.org wrote:
>
> See below 6 comments.
>
>> 21 марта 2018 г., в 2:48, Nikita Pettik <korablev at tarantool.org <mailto: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.
I need it as an analogue of process_rw(), since process_rw() is unavailable
via public BOX interface (declared as static). If you approve to make it public,
where should I place it and should I rename it?
Or, for instance, make sql_execute_dml be complete copy of process_rw()?
It is unlikely to be useful somewhere except for SQL (and box internals surely).
Initial indent was to reduce overhead while calling box functions from SQL bindings (sql.c):
ordinary call stack looks like: box_insert() -> box_process1() -> space lookup + process_rw().
In such scheme we also process redundant space lookup since we already have struct space * in cursor.
(Patch where I introduced this function isn’t in trunk, so I can easily fix it).
>>
>> @@ -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?
It might, but it is only applied for _sequence space during DDL routine
(and this is also answer for the first question: format of system space doesn’t change frequently),
so I decided to remove cursor/index overhead (as a part of overall DDL refactoring).
Probably, I should also rename this opcode to make it more clear.
>
>> {
>> 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.
It is used in the next patch. I don’t want to make a lot of diffs, since introducing of any opcodes
leads to auto-generating of opcodes.c/h and as a result to vast amount of redundant diff.
As for your example, during SQL DDL we need to execute queries such as:
'DELETE FROM BOX_SEQUENCE WHERE name = zName;’
And ‘name’ is not a key. Hence, we have to open cursor and iterate through the space.
But if we used pointers to spaces which were fetched at compile state,
they would become expired at VDBE runtime:
after first stage of DDL (e.g. deletion from _trigger), schema may change.
Moreover, we are going to support syntax like: ‘CREATE TABLE AS SELECT …’,
where DDL and DML are mixing. Thus we need facility to make space lookup at VDBE runtime.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180322/a2f1496b/attachment.html>
More information about the Tarantool-patches
mailing list