On 22 Mar 2018, at 14:42, v.shpilevoy@tarantool.org wrote:

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.