[tarantool-patches] [PATCH 0/4] Remove space id and index id from cursor

[tarantool-patches] [PATCH 0/4] Remove space id and index id from cursor

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/tree/np/gh-3122-remove-pgnoRoot
Issue: https://github.com/tarantool/tarantool/issues/3122

Before this patch, there was separate Tarantool specific cursor
alongside with "ordinary" SQLite one. However, now only Tarantool
spaces exist, so these two cursors can be merged into one.
Moreover, there is no need to hold in cursor space and index id,
since each appeal to them through cursor results in space lookup
via BOX API. To avoid this overhead, pointers to space and index
are stored in cursor (instead of ids) and DML executor is called
explicitly.

Nikita Pettik (4):
  Move space_is_system helper from CPP define guard
  sql: rework OP_Clear internals
  sql: remove struct ta_cursor and refactor BtCursor
  sql: replace pgnoRoot with struct space in BtCursor

 src/box/schema.h           |   6 +-
 src/box/sql.c              | 402 ++++++++++++++++++---------------------------
 src/box/sql/cursor.c       |  14 +-
 src/box/sql/cursor.h       |  16 +-
 src/box/sql/opcodes.c      |   2 +-
 src/box/sql/opcodes.h      |   2 +-
 src/box/sql/tarantoolInt.h |   4 +-
 src/box/sql/vdbe.c         |  63 +++----
 src/box/sql/vdbeInt.h      |   1 -
 9 files changed, 206 insertions(+), 304 deletions(-)

-- 
2.15.1

[tarantool-patches] [PATCH 1/4] Move space_is_system helper from CPP define guard

[Nikita Pettik]

It uses no C++ functionality, so should be available from C source
files.
---
 src/box/schema.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/box/schema.h b/src/box/schema.h
index 9df4d4e24..b612b8b6f 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -92,6 +92,9 @@ space_foreach(int (*func)(struct space *sp, void *udata), void *udata);
 const char *
 schema_find_name(enum schema_object_type type, uint32_t object_id);
 
+bool
+space_is_system(struct space *space);
+
 #if defined(__cplusplus)
 } /* extern "C" */
 
@@ -116,9 +119,6 @@ space_cache_replace(struct space *space);
 struct space *
 space_cache_delete(uint32_t id);
 
-bool
-space_is_system(struct space *space);
-
 void
 schema_init();
 
-- 
2.15.1

[tarantool-patches] [PATCH 2/4] sql: rework OP_Clear internals

[Nikita Pettik]

This patch makes OP_Clear use pointer to space instead of space id.
Moreover, in order to avoid excess function calls (such as box_delete() ->
box_process1(), which in its turn makes additional space lookup),
sql_execute_dml() function is introduced. It is an analogue of
process_rw() from BOX internals. Its purpose is to handle transaction
routine and call DML executor. This function will be also used later as
well during insertion and deletion.

Part of #3122
---
 src/box/sql.c              | 54 +++++++++++++++++++++++++++++++---------------
 src/box/sql/opcodes.c      |  2 +-
 src/box/sql/opcodes.h      |  2 +-
 src/box/sql/tarantoolInt.h |  2 +-
 src/box/sql/vdbe.c         | 28 ++++++++++--------------
 5 files changed, 51 insertions(+), 37 deletions(-)

diff --git a/src/box/sql.c b/src/box/sql.c
index 256985793..4f8b39810 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -191,6 +191,27 @@ is_tarantool_error(int rc)
 		rc == SQL_TARANTOOL_INSERT_FAIL);
 }
 
+/**
+ * This function is analogue of process_rw() from box module.
+ * Apart of calling space_execute_dml(), it handles transaction
+ * routine.
+ */
+static int
+sql_execute_dml(struct request *request, struct space *space)
+{
+	struct txn *txn = txn_begin_stmt(space);
+	if (txn == NULL)
+		return -1;
+	struct tuple *unused = NULL;
+	if (space_execute_dml(space, txn, request, &unused) != 0) {
+		txn_rollback_stmt();
+		return -1;
+	}
+	if (txn_commit_stmt(txn, request) != 0)
+		return -1;
+	return 0;
+}
+
 int tarantoolSqlite3CloseCursor(BtCursor *pCur)
 {
 	assert(pCur->curFlags & BTCF_TaCursor ||
@@ -659,10 +680,8 @@ int tarantoolSqlite3EphemeralClearTable(BtCursor *pCur)
 /*
  * Removes all instances from table.
  */
-int tarantoolSqlite3ClearTable(int iTable)
+int tarantoolSqlite3ClearTable(struct space *space)
 {
-	int space_id = SQLITE_PAGENO_TO_SPACEID(iTable);
-
 	/*
 	 *  There are two cases when we have to delete tuples one by one:
 	 *  1. When we are inside of another transaction, we can not use
@@ -670,30 +689,31 @@ int tarantoolSqlite3ClearTable(int iTable)
 	 *  2. Truncate on system spaces is disallowed. (because of triggers)
 	 *   (main usecase is _sql_stat4 table editing)
 	 */
-	if (box_txn() || space_id < BOX_SYSTEM_ID_MAX) {
-		int primary_index_id = 0;
-		char *key;
+	if (box_txn() || space_is_system(space)) {
 		uint32_t key_size;
 		box_tuple_t *tuple;
 		int rc;
-		box_iterator_t *iter;
-		iter = box_index_iterator(space_id, primary_index_id, ITER_ALL,
-					  nil_key, nil_key + sizeof(nil_key));
+		struct request request;
+		memset(&request, 0, sizeof(request));
+		request.type = IPROTO_DELETE;
+		request.space_id = space->def->id;
+		struct index *pk = space_index(space, 0 /* PK */);
+		struct iterator *iter =
+			index_create_iterator(pk, ITER_ALL, nil_key, 0);
 		if (iter == NULL)
 			return SQL_TARANTOOL_ITERATOR_FAIL;
-		while (box_iterator_next(iter, &tuple) == 0 && tuple != NULL) {
-			key = tuple_extract_key(tuple,
-						box_iterator_key_def(iter),
+		while (iterator_next(iter, &tuple) == 0 && tuple != NULL) {
+			request.key = tuple_extract_key(tuple, pk->def->key_def,
 						&key_size);
-			rc = box_delete(space_id, primary_index_id, key,
-					key + key_size, NULL);
+			request.key_end = request.key + key_size;
+			rc = sql_execute_dml(&request, space);
 			if (rc != 0) {
-				box_iterator_free(iter);
+				iterator_delete(iter);
 				return SQL_TARANTOOL_DELETE_FAIL;
 			}
 		}
-		box_iterator_free(iter);
-	} else if (box_truncate(space_id) != 0) {
+		iterator_delete(iter);
+	} else if (box_truncate(space->def->id) != 0) {
 		return SQL_TARANTOOL_DELETE_FAIL;
 	}
 
diff --git a/src/box/sql/opcodes.c b/src/box/sql/opcodes.c
index 53603037d..7a40b28a8 100644
--- a/src/box/sql/opcodes.c
+++ b/src/box/sql/opcodes.c
@@ -129,7 +129,7 @@ const char *sqlite3OpcodeName(int i){
     /* 115 */ "Real"             OpHelp("r[P2]=P4"),
     /* 116 */ "IdxInsert"        OpHelp("key=r[P2]"),
     /* 117 */ "IdxDelete"        OpHelp("key=r[P2@P3]"),
-    /* 118 */ "Clear"            OpHelp(""),
+    /* 118 */ "Clear"            OpHelp("space id = P1"),
     /* 119 */ "ResetSorter"      OpHelp(""),
     /* 120 */ "ParseSchema2"     OpHelp("rows=r[P1@P2]"),
     /* 121 */ "ParseSchema3"     OpHelp("name=r[P1] sql=r[P1+1]"),
diff --git a/src/box/sql/opcodes.h b/src/box/sql/opcodes.h
index 1fbb6b690..af2ba1963 100644
--- a/src/box/sql/opcodes.h
+++ b/src/box/sql/opcodes.h
@@ -118,7 +118,7 @@
 #define OP_Real          115 /* same as TK_FLOAT, synopsis: r[P2]=P4       */
 #define OP_IdxInsert     116 /* synopsis: key=r[P2]                        */
 #define OP_IdxDelete     117 /* synopsis: key=r[P2@P3]                     */
-#define OP_Clear         118
+#define OP_Clear         118 /* synopsis: space id = P1                    */
 #define OP_ResetSorter   119
 #define OP_ParseSchema2  120 /* synopsis: rows=r[P1@P2]                    */
 #define OP_ParseSchema3  121 /* synopsis: name=r[P1] sql=r[P1+1]           */
diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
index 39fdbcd76..c98871cd5 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -77,7 +77,7 @@ int tarantoolSqlite3Count(BtCursor * pCur, i64 * pnEntry);
 int tarantoolSqlite3Insert(BtCursor * pCur);
 int tarantoolSqlite3Replace(BtCursor * pCur);
 int tarantoolSqlite3Delete(BtCursor * pCur, u8 flags);
-int tarantoolSqlite3ClearTable(int iTable);
+int tarantoolSqlite3ClearTable(struct space *space);
 
 /* Rename table pTab with zNewName by inserting new tuple to _space.
  * SQL statement, which creates table with new name is saved in pzSqlStmt.
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 49ce51096..7de9b67c1 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4590,26 +4590,20 @@ case OP_IdxGE:  {       /* jump */
 	break;
 }
 
-/* Opcode: Clear P1 P2 P3
+/* Opcode: Clear P1 * * * *
+ * Synopsis: space id = P1
  *
- * Delete all contents of the database table or index whose root page
- * in the database file is given by P1.  But, unlike Destroy, do not
- * remove the table or index from the database file.
- *
- * The table being clear is in the main database file if P2==0.  If
- * P2==1 then the table to be clear is in the auxiliary database file
- * that is used to store tables create using CREATE TEMPORARY TABLE.
- *
- * If the P3 value is non-zero, then the table referred to must be an
- * intkey table (an SQL table, not an index). In this case the row change
- * count is incremented by the number of rows in the table being cleared.
- * If P3 is greater than zero, then the value stored in register P3 is
- * also incremented by the number of rows in the table being cleared.
- *
- * See also: Destroy
+ * Delete all contents of the space, which space id is given
+ * in P1 argument. Notice, that execution of this opcode inside
+ * active transaction would slow down perfomance, since it
+ * becomes impossible to use truncate feature.
  */
 case OP_Clear: {
-	rc = tarantoolSqlite3ClearTable(pOp->p1);
+	assert(pOp->p1 > 0);
+	uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(pOp->p1);
+	struct space *space = space_by_id(space_id);
+	assert(space != NULL);
+	rc = tarantoolSqlite3ClearTable(space);
 	if (rc) goto abort_due_to_error;
 	break;
 }
-- 
2.15.1

[tarantool-patches] [PATCH 3/4] sql: remove struct ta_cursor and refactor BtCursor

[Nikita Pettik]

ta_cursor cursor served as a wrapper around Tarantool specific members
which help to iterate through space. As far as now only Tarantool spaces
exist, there is no need in separate cursor. Thus, all such members are
transfered directly to BtCursor. Also, all useless field from BtCursor
are removed.

Part of #3122
---
 src/box/sql.c        | 275 +++++++++++++++++----------------------------------
 src/box/sql/cursor.c |  14 +--
 src/box/sql/cursor.h |  13 +--
 src/box/sql/vdbe.c   |  11 +--
 4 files changed, 101 insertions(+), 212 deletions(-)

diff --git a/src/box/sql.c b/src/box/sql.c
index 4f8b39810..4db253ff4 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -152,22 +152,8 @@ sql_get()
  * are accurately positioned, hence both 0 and 1 are fine.
  */
 
-/*
- * Tarantool iterator API was apparently designed by space aliens.
- * This wrapper is necessary for interfacing with the SQLite btree code.
- */
-struct ta_cursor {
-	size_t             size;
-	box_iterator_t    *iter;
-	struct tuple      *tuple_last;
-	enum iterator_type type;
-	/* Used only by ephemeral spaces, for ordinary space == NULL. */
-	struct space      *ephem_space;
-	char               key[1];
-};
-
-static struct ta_cursor *
-cursor_create(struct ta_cursor *c, size_t key_size);
+void
+key_alloc(BtCursor *c, size_t key_size);
 
 static int
 cursor_seek(BtCursor *pCur, int *pRes);
@@ -217,17 +203,10 @@ int tarantoolSqlite3CloseCursor(BtCursor *pCur)
 	assert(pCur->curFlags & BTCF_TaCursor ||
 	       pCur->curFlags & BTCF_TEphemCursor);
 
-	struct ta_cursor *c = pCur->pTaCursor;
-
-	pCur->pTaCursor = NULL;
-
-	if (c) {
-		if (c->iter)
-			box_iterator_free(c->iter);
-		if (c->tuple_last)
-			box_tuple_unref(c->tuple_last);
-		free(c);
-	}
+	if (pCur->iter)
+		box_iterator_free(pCur->iter);
+	if (pCur->last_tuple)
+		box_tuple_unref(pCur->last_tuple);
 	return SQLITE_OK;
 }
 
@@ -235,14 +214,10 @@ const void *tarantoolSqlite3PayloadFetch(BtCursor *pCur, u32 *pAmt)
 {
 	assert(pCur->curFlags & BTCF_TaCursor ||
 	       pCur->curFlags & BTCF_TEphemCursor);
+	assert(pCur->last_tuple != NULL);
 
-	struct ta_cursor *c = pCur->pTaCursor;
-
-	assert(c);
-	assert(c->tuple_last);
-
-	*pAmt = box_tuple_bsize(c->tuple_last);
-	return tuple_data(c->tuple_last);
+	*pAmt = box_tuple_bsize(pCur->last_tuple);
+	return tuple_data(pCur->last_tuple);
 }
 
 const void *
@@ -250,16 +225,14 @@ tarantoolSqlite3TupleColumnFast(BtCursor *pCur, u32 fieldno, u32 *field_size)
 {
 	assert(pCur->curFlags & BTCF_TaCursor ||
 	       pCur->curFlags & BTCF_TEphemCursor);
+	assert(pCur->last_tuple != NULL);
 
-	struct ta_cursor *c = pCur->pTaCursor;
-	assert(c != NULL);
-	assert(c->tuple_last != NULL);
-	struct tuple_format *format = tuple_format(c->tuple_last);
+	struct tuple_format *format = tuple_format(pCur->last_tuple);
 	assert(format->exact_field_count == 0
 	       || fieldno < format->exact_field_count);
 	if (format->fields[fieldno].offset_slot == TUPLE_OFFSET_SLOT_NIL)
 		return NULL;
-	const char *field = tuple_field(c->tuple_last, fieldno);
+	const char *field = tuple_field(pCur->last_tuple, fieldno);
 	const char *end = field;
 	mp_next(&end);
 	*field_size = end - field;
@@ -272,30 +245,26 @@ tarantoolSqlite3TupleColumnFast(BtCursor *pCur, u32 fieldno, u32 *field_size)
  */
 int tarantoolSqlite3First(BtCursor *pCur, int *pRes)
 {
-	struct ta_cursor *c = pCur->pTaCursor;
-	c = cursor_create(c, sizeof(nil_key));
-	if (!c) {
+	key_alloc(pCur, sizeof(nil_key));
+	if (pCur->key == NULL) {
 		*pRes = 1;
 		return SQLITE_NOMEM;
 	}
-	c->key[0] = nil_key[0];
-	c->type = ITER_GE;
-	pCur->pTaCursor = c;
+	pCur->key[0] = nil_key[0];
+	pCur->iter_type = ITER_GE;
 	return cursor_seek(pCur, pRes);
 }
 
 /* Set cursor to the last tuple in given space. */
 int tarantoolSqlite3Last(BtCursor *pCur, int *pRes)
 {
-	struct ta_cursor *c = pCur->pTaCursor;
-	c = cursor_create(c, sizeof(nil_key));
-	if (!c) {
+	key_alloc(pCur, sizeof(nil_key));
+	if (pCur->key == NULL) {
 		*pRes = 1;
 		return SQLITE_NOMEM;
 	}
-	c->key[0] = nil_key[0];
-	c->type = ITER_LE;
-	pCur->pTaCursor = c;
+	pCur->key[0] = nil_key[0];
+	pCur->iter_type = ITER_LE;
 	return cursor_seek(pCur, pRes);
 }
 
@@ -312,9 +281,7 @@ int tarantoolSqlite3Next(BtCursor *pCur, int *pRes)
 		*pRes = 1;
 		return SQLITE_OK;
 	}
-	assert(pCur->pTaCursor);
-	assert(iterator_direction(
-		((struct ta_cursor *)pCur->pTaCursor)->type) > 0);
+	assert(iterator_direction(pCur->iter_type) > 0);
 	return cursor_advance(pCur, pRes);
 }
 
@@ -329,9 +296,7 @@ int tarantoolSqlite3Previous(BtCursor *pCur, int *pRes)
 		*pRes = 1;
 		return SQLITE_OK;
 	}
-	assert(pCur->pTaCursor);
-	assert(iterator_direction(
-		((struct ta_cursor *)pCur->pTaCursor)->type) < 0);
+	assert(iterator_direction(pCur->iter_type) < 0);
 	return cursor_advance(pCur, pRes);
 }
 
@@ -340,12 +305,12 @@ int tarantoolSqlite3MovetoUnpacked(BtCursor *pCur, UnpackedRecord *pIdxKey,
 {
 	int rc, res_success;
 	size_t ks;
-	struct ta_cursor *taCur = pCur->pTaCursor;
 
-	ks = sqlite3VdbeMsgpackRecordLen(pIdxKey->aMem,
-					 pIdxKey->nField);
-	taCur = cursor_create(taCur, ks);
-	sqlite3VdbeMsgpackRecordPut((u8 *)taCur->key, pIdxKey->aMem,
+	ks = sqlite3VdbeMsgpackRecordLen(pIdxKey->aMem, pIdxKey->nField);
+	key_alloc(pCur, ks);
+	if (pCur->key == NULL)
+		return SQLITE_NOMEM;
+	sqlite3VdbeMsgpackRecordPut((u8 *)pCur->key, pIdxKey->aMem,
 				    pIdxKey->nField);
 
 	switch (pIdxKey->opcode) {
@@ -355,36 +320,34 @@ int tarantoolSqlite3MovetoUnpacked(BtCursor *pCur, UnpackedRecord *pIdxKey,
 	case 255:
 	/* Restore saved state. Just re-seek cursor.
 	   TODO: replace w/ named constant.  */
-		taCur->type = ((struct ta_cursor *)pCur->pTaCursor)->type;
 		res_success = 0;
 		break;
 	case OP_SeekLT:
-		taCur->type = ITER_LT;
+		pCur->iter_type = ITER_LT;
 		res_success = -1; /* item<key */
 		break;
 	case OP_SeekLE:
-		taCur->type = (pCur->hints & BTREE_SEEK_EQ) ?
-			      ITER_REQ : ITER_LE;
+		pCur->iter_type = (pCur->hints & BTREE_SEEK_EQ) ?
+				  ITER_REQ : ITER_LE;
 		res_success = 0; /* item==key */
 		break;
 	case OP_SeekGE:
-		taCur->type = (pCur->hints & BTREE_SEEK_EQ) ?
-			      ITER_EQ : ITER_GE;
+		pCur->iter_type = (pCur->hints & BTREE_SEEK_EQ) ?
+				  ITER_EQ : ITER_GE;
 		res_success = 0; /* item==key */
 		break;
 	case OP_SeekGT:
-		taCur->type = ITER_GT;
+		pCur->iter_type = ITER_GT;
 		res_success = 1; /* item>key */
 		break;
 	case OP_NoConflict:
 	case OP_NotFound:
 	case OP_Found:
 	case OP_IdxDelete:
-		taCur->type = ITER_EQ;
+		pCur->iter_type = ITER_EQ;
 		res_success = 0;
 		break;
 	}
-	pCur->pTaCursor = taCur;
 	rc = cursor_seek(pCur, pRes);
 	if (*pRes == 0) {
 		*pRes = res_success;
@@ -416,11 +379,7 @@ int tarantoolSqlite3EphemeralCount(struct BtCursor *pCur, i64 *pnEntry)
 {
 	assert(pCur->curFlags & BTCF_TEphemCursor);
 
-	struct ta_cursor *c = pCur->pTaCursor;
-	assert(c);
-	assert(c->ephem_space);
-
-	struct index *primary_index = *c->ephem_space->index;
+	struct index *primary_index = space_index(pCur->space, 0 /* PK */);
 	*pnEntry = index_size(primary_index);
 	return SQLITE_OK;
 }
@@ -486,14 +445,12 @@ int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
 		diag_log();
 		return SQL_TARANTOOL_ERROR;
 	}
-	struct ta_cursor *c = NULL;
-	c = cursor_create(c, field_count /* key size */);
-	if (!c) {
+	key_alloc(pCur, field_count /* key size */);
+	if (pCur->key == NULL) {
 		space_delete(ephemer_new_space);
 		return SQLITE_NOMEM;
 	}
-	c->ephem_space = ephemer_new_space;
-	pCur->pTaCursor = c;
+	pCur->space = ephemer_new_space;
 
 	int unused;
 	return tarantoolSqlite3First(pCur, &unused);
@@ -513,14 +470,10 @@ int tarantoolSqlite3EphemeralInsert(BtCursor *pCur)
 {
 	assert(pCur);
 	assert(pCur->curFlags & BTCF_TEphemCursor);
-	struct ta_cursor *c = pCur->pTaCursor;
-	assert(c);
-	assert(c->ephem_space);
-	mp_tuple_assert(pCur->pKey, pCur->pKey + pCur->nKey);
-
-	struct space *space = c->ephem_space;
-	if (space_ephemeral_replace(space, pCur->pKey,
-				    pCur->pKey + pCur->nKey) != 0) {
+	mp_tuple_assert(pCur->key, pCur->key + pCur->nKey);
+
+	if (space_ephemeral_replace(pCur->space, pCur->key,
+				    pCur->key + pCur->nKey) != 0) {
 		diag_log();
 		return SQL_TARANTOOL_INSERT_FAIL;
 	}
@@ -532,11 +485,7 @@ int tarantoolSqlite3EphemeralDrop(BtCursor *pCur)
 {
 	assert(pCur);
 	assert(pCur->curFlags & BTCF_TEphemCursor);
-
-	struct ta_cursor *c = pCur->pTaCursor;
-	assert(c->ephem_space);
-	space_delete_ephemeral(c->ephem_space);
-
+	space_delete_ephemeral(pCur->space);
 	return SQLITE_OK;
 }
 
@@ -549,12 +498,12 @@ static int insertOrReplace(BtCursor *pCur, int operationType)
 	int space_id = SQLITE_PAGENO_TO_SPACEID(pCur->pgnoRoot);
 	int rc;
 	if (operationType == TARANTOOL_INDEX_INSERT) {
-		rc = box_insert(space_id, pCur->pKey,
-				(const char *)pCur->pKey + pCur->nKey,
+		rc = box_insert(space_id, pCur->key,
+				(const char *)pCur->key + pCur->nKey,
 				NULL /* result */);
 	} else {
-		rc = box_replace(space_id, pCur->pKey,
-				 (const char *)pCur->pKey + pCur->nKey,
+		rc = box_replace(space_id, pCur->key,
+				 (const char *)pCur->key + pCur->nKey,
 				 NULL /* result */);
 	}
 
@@ -582,23 +531,18 @@ int tarantoolSqlite3Replace(BtCursor *pCur)
 int tarantoolSqlite3EphemeralDelete(BtCursor *pCur)
 {
 	assert(pCur->curFlags & BTCF_TEphemCursor);
-	assert(pCur->pTaCursor);
-	struct ta_cursor *c = pCur->pTaCursor;
-	struct space *ephem_space = c->ephem_space;
-	assert(ephem_space);
+	assert(pCur->iter != NULL);
+	assert(pCur->last_tuple != NULL);
 
 	char *key;
 	uint32_t key_size;
-	assert(c->iter);
-	assert(c->tuple_last);
-
-	key = tuple_extract_key(c->tuple_last,
-				box_iterator_key_def(c->iter),
+	key = tuple_extract_key(pCur->last_tuple,
+				box_iterator_key_def(pCur->iter),
 				&key_size);
 	if (key == NULL)
 		return SQL_TARANTOOL_DELETE_FAIL;
 
-	int rc = space_ephemeral_delete(ephem_space, key);
+	int rc = space_ephemeral_delete(pCur->space, key);
 	if (rc != 0) {
 		diag_log();
 		return SQL_TARANTOOL_DELETE_FAIL;
@@ -611,21 +555,18 @@ int tarantoolSqlite3Delete(BtCursor *pCur, u8 flags)
 	(void)flags;
 
 	assert(pCur->curFlags & BTCF_TaCursor);
+	assert(pCur->iter != NULL);
+	assert(pCur->last_tuple != NULL);
 
-	struct ta_cursor *c = pCur->pTaCursor;
 	uint32_t space_id, index_id;
 	char *key;
 	uint32_t key_size;
 	int rc;
 
-	assert(c);
-	assert(c->iter);
-	assert(c->tuple_last);
-
 	space_id = SQLITE_PAGENO_TO_SPACEID(pCur->pgnoRoot);
 	index_id = SQLITE_PAGENO_TO_INDEXID(pCur->pgnoRoot);
-	key = tuple_extract_key(c->tuple_last,
-				box_iterator_key_def(c->iter),
+	key = tuple_extract_key(pCur->last_tuple,
+				box_iterator_key_def(pCur->iter),
 				&key_size);
 	if (key == NULL)
 		return SQL_TARANTOOL_DELETE_FAIL;
@@ -648,11 +589,8 @@ int tarantoolSqlite3EphemeralClearTable(BtCursor *pCur)
 {
 	assert(pCur);
 	assert(pCur->curFlags & BTCF_TEphemCursor);
-	struct ta_cursor *c = pCur->pTaCursor;
-	assert(c->ephem_space);
 
-	struct space *ephem_space = c->ephem_space;
-	struct iterator *it = index_create_iterator(*ephem_space->index,
+	struct iterator *it = index_create_iterator(*pCur->space->index,
 						    ITER_ALL, nil_key,
 						    0 /* part_count */);
 	if (it == NULL) {
@@ -667,7 +605,7 @@ int tarantoolSqlite3EphemeralClearTable(BtCursor *pCur)
 	while (iterator_next(it, &tuple) == 0 && tuple != NULL) {
 		key = tuple_extract_key(tuple, box_iterator_key_def(it),
 					&key_size);
-		if (space_ephemeral_delete(ephem_space, key) != 0) {
+		if (space_ephemeral_delete(pCur->space, key) != 0) {
 			iterator_delete(it);
 			return SQL_TARANTOOL_DELETE_FAIL;
 		}
@@ -1025,8 +963,9 @@ int tarantoolSqlite3IdxKeyCompare(BtCursor *pCur, UnpackedRecord *pUnpacked,
 				  int *res)
 {
 	assert(pCur->curFlags & BTCF_TaCursor);
+	assert(pCur->iter != NULL);
+	assert(pCur->last_tuple != NULL);
 
-	struct ta_cursor *c = pCur->pTaCursor;
 	const box_key_def_t *key_def;
 	const struct tuple *tuple;
 	const char *base;
@@ -1042,13 +981,9 @@ int tarantoolSqlite3IdxKeyCompare(BtCursor *pCur, UnpackedRecord *pUnpacked,
 	uint32_t key_size;
 #endif
 
-	assert(c);
-	assert(c->iter);
-	assert(c->tuple_last);
-
-	key_def = box_iterator_key_def(c->iter);
+	key_def = box_iterator_key_def(pCur->iter);
 	n = MIN(pUnpacked->nField, key_def->part_count);
-	tuple = c->tuple_last;
+	tuple = pCur->last_tuple;
 	base = tuple_data(tuple);
 	format = tuple_format(tuple);
 	field_map = tuple_field_map(tuple);
@@ -1118,6 +1053,7 @@ out:
  */
 int tarantoolSqlite3IncrementMaxid(BtCursor *pCur)
 {
+	assert(pCur->curFlags & BTCF_TaCursor);
 	/* ["max_id"] */
 	static const char key[] = {
 		(char)0x91, /* MsgPack array(1) */
@@ -1134,9 +1070,6 @@ int tarantoolSqlite3IncrementMaxid(BtCursor *pCur)
 		1           /* MsgPack int(1) */
 	};
 
-	assert(pCur->curFlags & BTCF_TaCursor);
-
-	struct ta_cursor *c = pCur->pTaCursor;
 	uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(pCur->pgnoRoot);
 	uint32_t index_id = SQLITE_PAGENO_TO_INDEXID(pCur->pgnoRoot);
 	box_tuple_t *res;
@@ -1150,54 +1083,30 @@ int tarantoolSqlite3IncrementMaxid(BtCursor *pCur)
 	if (rc != 0 || res == NULL) {
 		return SQL_TARANTOOL_ERROR;
 	}
-	if (!c) {
-		c = cursor_create(NULL, 0);
-		if (!c) return SQLITE_NOMEM;
-		pCur->pTaCursor = c;
-		c->type = ITER_EQ; /* store some meaningfull value */
-	} else if (c->tuple_last) {
-		box_tuple_unref(c->tuple_last);
+	if (pCur->last_tuple != NULL) {
+		box_tuple_unref(pCur->last_tuple);
 	}
 	box_tuple_ref(res);
-	c->tuple_last = res;
+	pCur->last_tuple = res;
 	pCur->eState = CURSOR_VALID;
 	return SQLITE_OK;
 }
 
 /*
- * Allocate or grow cursor.
+ * Allocate or grow memory for cursor's key.
  * Result->type value is unspecified.
  */
-static struct ta_cursor *
-cursor_create(struct ta_cursor *c, size_t key_size)
+void
+key_alloc(BtCursor *cur, size_t key_size)
 {
-	size_t             size;
-	struct ta_cursor  *res;
-	struct space *ephem_space;
-
-	if (c) {
-		size = c->size;
-		ephem_space = c->ephem_space;
-		if (size - offsetof(struct ta_cursor, key) >= key_size)
-			return c;
+	if (cur->key == NULL) {
+		cur->key = malloc(key_size);
+		cur->iter = NULL;
+		cur->last_tuple = NULL;
 	} else {
-		size = sizeof(*c);
-		ephem_space = NULL;
+		cur->key = realloc(cur->key, key_size);
 	}
-
-	while (size - offsetof(struct ta_cursor, key) < key_size)
-		size *= 2;
-
-	res = realloc(c, size);
-	if (res) {
-		res->size = size;
-		res->ephem_space = ephem_space;
-		if (!c) {
-			res->iter = NULL;
-			res->tuple_last = NULL;
-		}
-	}
-	return res;
+	cur->nKey = key_size;
 }
 
 /*
@@ -1215,13 +1124,10 @@ cursor_create(struct ta_cursor *c, size_t key_size)
 static int
 cursor_seek(BtCursor *pCur, int *pRes)
 {
-	struct ta_cursor *c = pCur->pTaCursor;
-	assert(c != NULL);
-
 	/* Close existing iterator, if any */
-	if (c->iter) {
-		box_iterator_free(c->iter);
-		c->iter = NULL;
+	if (pCur->iter) {
+		box_iterator_free(pCur->iter);
+		pCur->iter = NULL;
 	}
 
 	struct space *space;
@@ -1234,24 +1140,24 @@ cursor_seek(BtCursor *pCur, int *pRes)
 		uint32_t index_id = SQLITE_PAGENO_TO_INDEXID(pCur->pgnoRoot);
 		index = index_find(space, index_id);
 	} else {
-		space = c->ephem_space;
+		space = pCur->space;
 		index = *space->index;
 	}
 
-	const char *key = (const char *)c->key;
+	const char *key = (const char *)pCur->key;
 	uint32_t part_count = mp_decode_array(&key);
-	if (key_validate(index->def, c->type, key, part_count)) {
+	if (key_validate(index->def, pCur->iter_type, key, part_count)) {
 		diag_log();
 		return SQL_TARANTOOL_ITERATOR_FAIL;
 	}
 
-	struct iterator *it = index_create_iterator(index, c->type, key,
+	struct iterator *it = index_create_iterator(index, pCur->iter_type, key,
 						    part_count);
 	if (it == NULL) {
 		pCur->eState = CURSOR_INVALID;
 		return SQL_TARANTOOL_ITERATOR_FAIL;
 	}
-	c->iter = it;
+	pCur->iter = it;
 	pCur->eState = CURSOR_VALID;
 
 	return cursor_advance(pCur, pRes);
@@ -1270,16 +1176,15 @@ cursor_seek(BtCursor *pCur, int *pRes)
 static int
 cursor_advance(BtCursor *pCur, int *pRes)
 {
-	struct ta_cursor *c = pCur->pTaCursor;
-	assert(c);
-	assert(c->iter);
+	assert(pCur->iter != NULL);
 
 	struct tuple *tuple;
-	if (iterator_next(c->iter, &tuple) != 0)
+	if (iterator_next(pCur->iter, &tuple) != 0)
 		return SQL_TARANTOOL_ITERATOR_FAIL;
 	if (tuple != NULL && tuple_bless(tuple) == NULL)
 		return SQL_TARANTOOL_ITERATOR_FAIL;
-	if (c->tuple_last) box_tuple_unref(c->tuple_last);
+	if (pCur->last_tuple)
+		box_tuple_unref(pCur->last_tuple);
 	if (tuple) {
 		box_tuple_ref(tuple);
 		*pRes = 0;
@@ -1287,7 +1192,7 @@ cursor_advance(BtCursor *pCur, int *pRes)
 		pCur->eState = CURSOR_INVALID;
 		*pRes = 1;
 	}
-	c->tuple_last = tuple;
+	pCur->last_tuple = tuple;
 	return SQLITE_OK;
 }
 
@@ -1749,9 +1654,7 @@ sql_debug_info(struct info_handler *h)
 int tarantoolSqlite3EphemeralGetMaxId(BtCursor *pCur, uint32_t fieldno,
 				       uint64_t *max_id)
 {
-	assert(pCur->pTaCursor);
-	struct ta_cursor *c = pCur->pTaCursor;
-	struct space *ephem_space = c->ephem_space;
+	struct space *ephem_space = pCur->space;
 	assert(ephem_space);
 	struct index *primary_index = *ephem_space->index;
 
diff --git a/src/box/sql/cursor.c b/src/box/sql/cursor.c
index 8e6d5c82b..02add5f11 100644
--- a/src/box/sql/cursor.c
+++ b/src/box/sql/cursor.c
@@ -38,8 +38,10 @@
 void
 sqlite3ClearCursor(BtCursor * pCur)
 {
-	sqlite3_free(pCur->pKey);
-	pCur->pKey = 0;
+	sqlite3_free(pCur->key);
+	pCur->key = 0;
+	pCur->iter = NULL;
+	pCur->last_tuple = NULL;
 	pCur->eState = CURSOR_INVALID;
 }
 
@@ -55,16 +57,11 @@ sqlite3CursorHintFlags(BtCursor * pCur, unsigned x)
 
 /*
  * Initialize memory that will be converted into a BtCursor object.
- *
- * The simple approach here would be to memset() the entire object
- * to zero.  But it turns out that the apPage[] and aiIdx[] arrays
- * do not need to be zeroed and they are large, so we can save a lot
- * of run-time by skipping the initialization of those elements.
  */
 void
 sqlite3CursorZero(BtCursor * p)
 {
-	memset(p, 0, offsetof(BtCursor, hints));
+	memset(p, 0, sizeof(*p));
 }
 
 /*
@@ -165,7 +162,6 @@ sqlite3CursorMovetoUnpacked(BtCursor * pCur,	/* The cursor to be moved */
 {
 	assert(pRes);
 	assert(pIdxKey);
-	assert(pCur->pKeyInfo);
 	assert((pCur->curFlags & BTCF_TaCursor) ||
 	       (pCur->curFlags & BTCF_TEphemCursor));
 
diff --git a/src/box/sql/cursor.h b/src/box/sql/cursor.h
index 646d9913d..6b23ebf4a 100644
--- a/src/box/sql/cursor.h
+++ b/src/box/sql/cursor.h
@@ -58,19 +58,16 @@ typedef struct BtCursor BtCursor;
  * for ordinary space, or to TEphemCursor for ephemeral space.
  */
 struct BtCursor {
-	BtCursor *pNext;	/* Forms a linked list of all cursors */
 	i64 nKey;		/* Size of pKey, or last integer key */
-	void *pKey;		/* Saved key that was cursor last known position */
 	Pgno pgnoRoot;		/* Contains both space_id and index_id */
 	u8 curFlags;		/* zero or more BTCF_* flags defined below */
 	u8 eState;		/* One of the CURSOR_XXX constants (see below) */
 	u8 hints;		/* As configured by CursorSetHints() */
-	/* All fields above are zeroed when the cursor is allocated.  See
-	 * sqlite3CursorZero().  Fields that follow must be manually
-	 * initialized.
-	 */
-	struct KeyInfo *pKeyInfo;	/* Argument passed to comparison function */
-	void *pTaCursor;	/* Tarantool cursor */
+	struct space *space;
+	struct iterator *iter;
+	enum iterator_type iter_type;
+	struct tuple *last_tuple;
+	char *key;		/* Saved key that was cursor last known position */
 };
 
 void sqlite3CursorZero(BtCursor *);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7de9b67c1..cd3321c4f 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3214,10 +3214,8 @@ case OP_OpenWrite:
 	assert(p2 >= 1);
 	pBtCur = pCur->uc.pCursor;
 	pBtCur->pgnoRoot = p2;
-	pBtCur->pKeyInfo = pKeyInfo;
 	pBtCur->eState = CURSOR_INVALID;
 	pBtCur->curFlags |= BTCF_TaCursor;
-	pBtCur->pTaCursor = 0;
 	pCur->pKeyInfo = pKeyInfo;
 
 	open_cursor_set_hints:
@@ -3254,10 +3252,8 @@ case OP_OpenTEphemeral: {
 	pBtCur = pCx->uc.pCursor;
 	/* Ephemeral spaces don't have space_id */
 	pBtCur->pgnoRoot = 0;
-	pBtCur->pKeyInfo = pCx->pKeyInfo;
 	pBtCur->eState = CURSOR_INVALID;
 	pBtCur->curFlags = BTCF_TEphemCursor;
-	pBtCur->pTaCursor = 0;
 
 	rc = tarantoolSqlite3EphemeralCreate(pCx->uc.pCursor, pOp->p2,
 					     pOp->p4.pKeyInfo->aColl[0]);
@@ -4406,11 +4402,8 @@ case OP_IdxInsert: {        /* in2 */
 		rc = sqlite3VdbeSorterWrite(pC, pIn2);
 	} else {
 		BtCursor *pBtCur = pC->uc.pCursor;
-		assert((pIn2->z == 0) == (pBtCur->pKeyInfo == 0));
-
 		pBtCur->nKey = pIn2->n;
-		pBtCur->pKey = pIn2->z;
-
+		pBtCur->key = pIn2->z;
 		if (pBtCur->curFlags & BTCF_TaCursor) {
 			/* Make sure that memory has been allocated on region. */
 			assert(aMem[pOp->p2].flags & MEM_Ephem);
@@ -4435,7 +4428,7 @@ case OP_IdxInsert: {        /* in2 */
 		 * nullified.
 		 */
 		pBtCur->nKey = 0;
-		pBtCur->pKey = NULL;
+		pBtCur->key = NULL;
 	}
 
 	if (pOp->p5 & OPFLAG_OE_IGNORE) {
-- 
2.15.1

[tarantool-patches] [PATCH 4/4] sql: replace pgnoRoot with struct space in BtCursor

[Nikita Pettik]

Instead of passing encoded space id and index id to SQL bindings,
pointers to space and index are saved in cursor and passed implicitly.
Space and index lookups appear during execution of
OP_OpenRead/OP_OpenWrite once. Moreover, having struct space it has
become possible to remove several wrapper-function calls on insertions
and deletions by invoking sql_execute_dml().

Closes #3122
---
 src/box/sql.c              | 87 +++++++++++++++++++++++-----------------------
 src/box/sql/cursor.h       |  3 +-
 src/box/sql/tarantoolInt.h |  2 +-
 src/box/sql/vdbe.c         | 24 ++++++-------
 src/box/sql/vdbeInt.h      |  1 -
 5 files changed, 58 insertions(+), 59 deletions(-)

diff --git a/src/box/sql.c b/src/box/sql.c
index 4db253ff4..59b41186e 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -388,9 +388,7 @@ int tarantoolSqlite3Count(BtCursor *pCur, i64 *pnEntry)
 {
 	assert(pCur->curFlags & BTCF_TaCursor);
 
-	uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(pCur->pgnoRoot);
-	uint32_t index_id = SQLITE_PAGENO_TO_INDEXID(pCur->pgnoRoot);
-	*pnEntry = box_index_len(space_id, index_id);
+	*pnEntry = index_size(pCur->index);
 	return SQLITE_OK;
 }
 
@@ -451,6 +449,7 @@ int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
 		return SQLITE_NOMEM;
 	}
 	pCur->space = ephemer_new_space;
+	pCur->index = *ephemer_new_space->index;
 
 	int unused;
 	return tarantoolSqlite3First(pCur, &unused);
@@ -495,17 +494,19 @@ static int insertOrReplace(BtCursor *pCur, int operationType)
 	assert(operationType == TARANTOOL_INDEX_INSERT ||
 	       operationType == TARANTOOL_INDEX_REPLACE);
 
-	int space_id = SQLITE_PAGENO_TO_SPACEID(pCur->pgnoRoot);
 	int rc;
+	struct request request;
+	memset(&request, 0, sizeof(request));
+	request.tuple = pCur->key;
+	request.tuple_end = pCur->key + pCur->nKey;
+	request.space_id = pCur->space->def->id;
+	mp_tuple_assert(request.tuple, request.tuple_end);
 	if (operationType == TARANTOOL_INDEX_INSERT) {
-		rc = box_insert(space_id, pCur->key,
-				(const char *)pCur->key + pCur->nKey,
-				NULL /* result */);
+		request.type = IPROTO_INSERT;
 	} else {
-		rc = box_replace(space_id, pCur->key,
-				 (const char *)pCur->key + pCur->nKey,
-				 NULL /* result */);
+		request.type = IPROTO_REPLACE;
 	}
+	rc = sql_execute_dml(&request, pCur->space);
 
 	return rc == 0 ? SQLITE_OK : SQL_TARANTOOL_INSERT_FAIL;;
 }
@@ -558,20 +559,23 @@ int tarantoolSqlite3Delete(BtCursor *pCur, u8 flags)
 	assert(pCur->iter != NULL);
 	assert(pCur->last_tuple != NULL);
 
-	uint32_t space_id, index_id;
 	char *key;
 	uint32_t key_size;
 	int rc;
 
-	space_id = SQLITE_PAGENO_TO_SPACEID(pCur->pgnoRoot);
-	index_id = SQLITE_PAGENO_TO_INDEXID(pCur->pgnoRoot);
 	key = tuple_extract_key(pCur->last_tuple,
 				box_iterator_key_def(pCur->iter),
 				&key_size);
 	if (key == NULL)
 		return SQL_TARANTOOL_DELETE_FAIL;
 
-	rc = box_delete(space_id, index_id, key, key + key_size, NULL);
+	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);
 
 	return rc == 0 ? SQLITE_OK : SQL_TARANTOOL_DELETE_FAIL;
 }
@@ -1060,6 +1064,7 @@ int tarantoolSqlite3IncrementMaxid(BtCursor *pCur)
 		(char)0xa6, /* MsgPack string(6) */
 		'm', 'a', 'x', '_', 'i', 'd'
 	};
+
 	/* [["+", 1, 1]]*/
 	static const char ops[] = {
 		(char)0x91, /* MsgPack array(1) */
@@ -1070,17 +1075,26 @@ int tarantoolSqlite3IncrementMaxid(BtCursor *pCur)
 		1           /* MsgPack int(1) */
 	};
 
-	uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(pCur->pgnoRoot);
-	uint32_t index_id = SQLITE_PAGENO_TO_INDEXID(pCur->pgnoRoot);
-	box_tuple_t *res;
+	struct tuple *res;
 	int rc;
 
-	rc = box_update(space_id, index_id,
-		key, key + sizeof(key),
-		ops, ops + sizeof(ops),
-		0,
-		&res);
-	if (rc != 0 || res == NULL) {
+	struct iterator *it = index_create_iterator(pCur->index, ITER_ALL,
+						    &key[1], sizeof(key) - 1);
+	if (iterator_next(it, &res) != 0 || res == NULL) {
+		iterator_delete(it);
+		return SQL_TARANTOOL_ERROR;
+	}
+	struct request request;
+	memset(&request, 0, sizeof(request));
+	request.tuple = key;
+	request.tuple_end = key + sizeof(key);
+	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);
+	if (rc != 0) {
+		iterator_delete(it);
 		return SQL_TARANTOOL_ERROR;
 	}
 	if (pCur->last_tuple != NULL) {
@@ -1089,6 +1103,7 @@ int tarantoolSqlite3IncrementMaxid(BtCursor *pCur)
 	box_tuple_ref(res);
 	pCur->last_tuple = res;
 	pCur->eState = CURSOR_VALID;
+	iterator_delete(it);
 	return SQLITE_OK;
 }
 
@@ -1129,30 +1144,15 @@ cursor_seek(BtCursor *pCur, int *pRes)
 		box_iterator_free(pCur->iter);
 		pCur->iter = NULL;
 	}
-
-	struct space *space;
-	struct index *index;
-	uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(pCur->pgnoRoot);
-	if (space_id != 0) {
-		space = space_cache_find(space_id);
-		if (space == NULL)
-			return SQL_TARANTOOL_ERROR;
-		uint32_t index_id = SQLITE_PAGENO_TO_INDEXID(pCur->pgnoRoot);
-		index = index_find(space, index_id);
-	} else {
-		space = pCur->space;
-		index = *space->index;
-	}
-
 	const char *key = (const char *)pCur->key;
 	uint32_t part_count = mp_decode_array(&key);
-	if (key_validate(index->def, pCur->iter_type, key, part_count)) {
+	if (key_validate(pCur->index->def, pCur->iter_type, key, part_count)) {
 		diag_log();
 		return SQL_TARANTOOL_ITERATOR_FAIL;
 	}
 
-	struct iterator *it = index_create_iterator(index, pCur->iter_type, key,
-						    part_count);
+	struct iterator *it = index_create_iterator(pCur->index, pCur->iter_type,
+						    key, part_count);
 	if (it == NULL) {
 		pCur->eState = CURSOR_INVALID;
 		return SQL_TARANTOOL_ITERATOR_FAIL;
@@ -1690,13 +1690,14 @@ int tarantoolSqlite3EphemeralGetMaxId(BtCursor *pCur, uint32_t fieldno,
  * If index is empty - return 0 in max_id and success status
  */
 int
-tarantoolSqlGetMaxId(uint32_t space_id, uint32_t index_id, uint32_t fieldno,
+tarantoolSqlGetMaxId(BtCursor *cur, uint32_t fieldno,
 		     uint64_t *max_id)
 {
 	char key[16];
 	struct tuple *tuple;
 	char *key_end = mp_encode_array(key, 0);
-	if (box_index_max(space_id, index_id, key, key_end, &tuple) != 0)
+	if (box_index_max(cur->space->def->id, cur->index->def->iid,
+			  key, key_end, &tuple) != 0)
 		return -1;
 
 	/* Index is empty  */
diff --git a/src/box/sql/cursor.h b/src/box/sql/cursor.h
index 6b23ebf4a..5b8e7c29d 100644
--- a/src/box/sql/cursor.h
+++ b/src/box/sql/cursor.h
@@ -32,7 +32,6 @@
 #ifndef SQLITE_CURSOR_H
 #define SQLITE_CURSOR_H
 
-typedef u32 Pgno;
 typedef struct BtCursor BtCursor;
 
 /*
@@ -59,11 +58,11 @@ typedef struct BtCursor BtCursor;
  */
 struct BtCursor {
 	i64 nKey;		/* Size of pKey, or last integer key */
-	Pgno pgnoRoot;		/* Contains both space_id and index_id */
 	u8 curFlags;		/* zero or more BTCF_* flags defined below */
 	u8 eState;		/* One of the CURSOR_XXX constants (see below) */
 	u8 hints;		/* As configured by CursorSetHints() */
 	struct space *space;
+	struct index *index;
 	struct iterator *iter;
 	enum iterator_type iter_type;
 	struct tuple *last_tuple;
diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
index c98871cd5..6f1ba3784 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -150,5 +150,5 @@ 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(uint32_t space_id, uint32_t index_id, uint32_t fieldno,
+int tarantoolSqlGetMaxId(BtCursor *cur, uint32_t fieldno,
 			 uint64_t * max_id);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index cd3321c4f..1ee8c4d7b 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3137,10 +3137,15 @@ case OP_ReopenIdx: {
 
 	assert(pOp->p5==0 || pOp->p5==OPFLAG_SEEKEQ);
 	assert(pOp->p4type==P4_KEYINFO);
-	pCur = p->apCsr[pOp->p1];
+	/*
+	 * FIXME: optimization temporary disabled until the whole
+	 * OP_Open* is reworked, i.e. pointer to space is passed
+	 * to this opcode instead of space id.
+	 */
+	/* pCur = p->apCsr[pOp->p1];
 	if (pCur && pCur->pgnoRoot==(u32)pOp->p2) {
 		goto open_cursor_set_hints;
-	}
+	} */
 	/* If the cursor is not currently open or is open on a different
 	 * index, then fall through into OP_OpenRead to force a reopen
 	 */
@@ -3209,16 +3214,17 @@ case OP_OpenWrite:
 	pCur = allocateCursor(p, pOp->p1, nField, CURTYPE_TARANTOOL);
 	if (pCur==0) goto no_mem;
 	pCur->nullRow = 1;
-	pCur->pgnoRoot = p2;
 
 	assert(p2 >= 1);
 	pBtCur = pCur->uc.pCursor;
-	pBtCur->pgnoRoot = p2;
+	pBtCur->space = space_by_id(SQLITE_PAGENO_TO_SPACEID(p2));
+	pBtCur->index = space_index(pBtCur->space, SQLITE_PAGENO_TO_INDEXID(p2));
+	assert(pBtCur->space != NULL && pBtCur->index != NULL);
 	pBtCur->eState = CURSOR_INVALID;
 	pBtCur->curFlags |= BTCF_TaCursor;
 	pCur->pKeyInfo = pKeyInfo;
 
-	open_cursor_set_hints:
+//	open_cursor_set_hints:
 	assert(OPFLAG_BULKCSR==BTREE_BULKLOAD);
 	assert(OPFLAG_SEEKEQ==BTREE_SEEK_EQ);
 	testcase( pOp->p5 & OPFLAG_BULKCSR);
@@ -3251,7 +3257,6 @@ case OP_OpenTEphemeral: {
 	pCx->pKeyInfo  = pOp->p4.pKeyInfo;
 	pBtCur = pCx->uc.pCursor;
 	/* Ephemeral spaces don't have space_id */
-	pBtCur->pgnoRoot = 0;
 	pBtCur->eState = CURSOR_INVALID;
 	pBtCur->curFlags = BTCF_TEphemCursor;
 
@@ -3791,7 +3796,6 @@ case OP_Sequence: {           /* out2 */
 case OP_NextId: {     /* out3 */
 	VdbeCursor *pC;    /* The VDBE cursor */
 	int p2;            /* Column number, which stores the id */
-	int pgno;          /* Page number of the cursor */
 	pC = p->apCsr[pOp->p1];
 	p2 = pOp->p2;
 	pOut = &aMem[pOp->p3];
@@ -3799,11 +3803,7 @@ case OP_NextId: {     /* out3 */
 	/* This opcode is Tarantool specific.  */
 	assert(pC->uc.pCursor->curFlags & BTCF_TaCursor);
 
-	pgno = pC->pgnoRoot;
-
-	tarantoolSqlGetMaxId(SQLITE_PAGENO_TO_SPACEID(pgno),
-			     SQLITE_PAGENO_TO_INDEXID(pgno),
-			     p2,
+	tarantoolSqlGetMaxId(pC->uc.pCursor, p2,
 			     (uint64_t *) &pOut->u.i);
 
 	pOut->u.i += 1;
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 1836673a4..8b622de5b 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -122,7 +122,6 @@ struct VdbeCursor {
 		VdbeSorter *pSorter;	/* CURTYPE_SORTER. Sorter object */
 	} uc;
 	KeyInfo *pKeyInfo;	/* Info about index keys needed by index cursors */
-	Pgno pgnoRoot;		/* Root page of the open cursor */
 	i16 nField;		/* Number of fields in the header */
 	u16 nHdrParsed;		/* Number of header fields parsed so far */
 	const u8 *aRow;		/* Data for the current row, if all on one page */
-- 
2.15.1

[tarantool-patches] Re: [PATCH 2/4] sql: rework OP_Clear internals

[Kirill Yukhin]

Hello,

My comments are inlined.

On 19 мар 21:10, Nikita Pettik wrote:
> This patch makes OP_Clear use pointer to space instead of space id.
> Moreover, in order to avoid excess function calls (such as box_delete() ->
> box_process1(), which in its turn makes additional space lookup),
> sql_execute_dml() function is introduced. It is an analogue of
> process_rw() from BOX internals. Its purpose is to handle transaction
> routine and call DML executor. This function will be also used later as
> well during insertion and deletion.
> 
> Part of #3122
> ---
>  src/box/sql.c              | 54 +++++++++++++++++++++++++++++++---------------
>  src/box/sql/opcodes.c      |  2 +-
>  src/box/sql/opcodes.h      |  2 +-
>  src/box/sql/tarantoolInt.h |  2 +-
>  src/box/sql/vdbe.c         | 28 ++++++++++--------------
>  5 files changed, 51 insertions(+), 37 deletions(-)
> 
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 256985793..4f8b39810 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -191,6 +191,27 @@ is_tarantool_error(int rc)
>  		rc == SQL_TARANTOOL_INSERT_FAIL);
>  }
>  
> +/**
> + * This function is analogue of process_rw() from box module.
> + * Apart of calling space_execute_dml(), it handles transaction
> + * routine.
> + */
Could you pls describe params as well?

> +static int
> +sql_execute_dml(struct request *request, struct space *space)
> +{
> +	struct txn *txn = txn_begin_stmt(space);
> +	if (txn == NULL)
> +		return -1;
> +	struct tuple *unused = NULL;
> +	if (space_execute_dml(space, txn, request, &unused) != 0) {
> +		txn_rollback_stmt();
> +		return -1;
> +	}
> +	if (txn_commit_stmt(txn, request) != 0)
> +		return -1;
> +	return 0;
> +}
So, you've copied process_rw, removing statitics udpate and access
checks? I think, this won't work. The only concern I see here is that
process_rw returns a tuple, which is useless for SQL so far.

> @@ -670,30 +689,31 @@ int tarantoolSqlite3ClearTable(int iTable)
>  			}
>  		}
> -		box_iterator_free(iter);
> -	} else if (box_truncate(space_id) != 0) {
> +		iterator_delete(iter);
> +	} else if (box_truncate(space->def->id) != 0) {
I think we should block box_truncate sp far: too much complications this
DDL operation introduce. We'll re-implement it as optimization in furture.
Also, we'll support ANSI SQL's TRUNCATE TABLE.

--
Kirill

[tarantool-patches] Re: [PATCH 4/4] sql: replace pgnoRoot with struct space in BtCursor

[Kirill Yukhin]

Hello,

Single comment.

On 19 мар 21:10, Nikita Pettik wrote:
> Instead of passing encoded space id and index id to SQL bindings,
> pointers to space and index are saved in cursor and passed implicitly.
> Space and index lookups appear during execution of
> OP_OpenRead/OP_OpenWrite once. Moreover, having struct space it has
> become possible to remove several wrapper-function calls on insertions
> and deletions by invoking sql_execute_dml().
> 
> Closes #3122
> ---
>  src/box/sql.c              | 87 +++++++++++++++++++++++-----------------------
>  src/box/sql/cursor.h       |  3 +-
>  src/box/sql/tarantoolInt.h |  2 +-
>  src/box/sql/vdbe.c         | 24 ++++++-------
>  src/box/sql/vdbeInt.h      |  1 -
>  5 files changed, 58 insertions(+), 59 deletions(-)
>
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index cd3321c4f..1ee8c4d7b 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -3209,16 +3214,17 @@ case OP_OpenWrite:
>  	pCur = allocateCursor(p, pOp->p1, nField, CURTYPE_TARANTOOL);
>  	if (pCur==0) goto no_mem;
>  	pCur->nullRow = 1;
> -	pCur->pgnoRoot = p2;
>  
>  	assert(p2 >= 1);
>  	pBtCur = pCur->uc.pCursor;
> -	pBtCur->pgnoRoot = p2;
> +	pBtCur->space = space_by_id(SQLITE_PAGENO_TO_SPACEID(p2));
> +	pBtCur->index = space_index(pBtCur->space, SQLITE_PAGENO_TO_INDEXID(p2));
> +	assert(pBtCur->space != NULL && pBtCur->index != NULL);
>  	pBtCur->eState = CURSOR_INVALID;
>  	pBtCur->curFlags |= BTCF_TaCursor;
>  	pCur->pKeyInfo = pKeyInfo;
>  
> -	open_cursor_set_hints:
> +//	open_cursor_set_hints:
Please, don't usse C++ style comments.

--
Thanks, Kirill

[tarantool-patches] Re: [PATCH 2/4] sql: rework OP_Clear internals

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 3092 bytes --]


> On 20 Mar 2018, at 13:54, Kirill Yukhin <kyukhin@tarantool.org> wrote:
> 
> Hello,
> 
> My comments are inlined.
> 
> On 19 мар 21:10, Nikita Pettik wrote:
>> This patch makes OP_Clear use pointer to space instead of space id.
>> Moreover, in order to avoid excess function calls (such as box_delete() ->
>> box_process1(), which in its turn makes additional space lookup),
>> sql_execute_dml() function is introduced. It is an analogue of
>> process_rw() from BOX internals. Its purpose is to handle transaction
>> routine and call DML executor. This function will be also used later as
>> well during insertion and deletion.
>> 
>> Part of #3122
>> ---
>> src/box/sql.c              | 54 +++++++++++++++++++++++++++++++---------------
>> src/box/sql/opcodes.c      |  2 +-
>> src/box/sql/opcodes.h      |  2 +-
>> src/box/sql/tarantoolInt.h |  2 +-
>> src/box/sql/vdbe.c         | 28 ++++++++++--------------
>> 5 files changed, 51 insertions(+), 37 deletions(-)
>> 
>> diff --git a/src/box/sql.c b/src/box/sql.c
>> index 256985793..4f8b39810 100644
>> --- a/src/box/sql.c
>> +++ b/src/box/sql.c
>> @@ -191,6 +191,27 @@ is_tarantool_error(int rc)
>> 		rc == SQL_TARANTOOL_INSERT_FAIL);
>> }
>> 
>> +/**
>> + * This function is analogue of process_rw() from box module.
>> + * Apart of calling space_execute_dml(), it handles transaction
>> + * routine.
>> + */
> Could you pls describe params as well?

As you wish, but it would look like “comments for the sake of comments”:
function is 10 lines long and I stick to the point that the purpose of 
arguments is obvious.  

> 
>> +static int
>> +sql_execute_dml(struct request *request, struct space *space)
>> +{
>> +	struct txn *txn = txn_begin_stmt(space);
>> +	if (txn == NULL)
>> +		return -1;
>> +	struct tuple *unused = NULL;
>> +	if (space_execute_dml(space, txn, request, &unused) != 0) {
>> +		txn_rollback_stmt();
>> +		return -1;
>> +	}
>> +	if (txn_commit_stmt(txn, request) != 0)
>> +		return -1;
>> +	return 0;
>> +}
> So, you've copied process_rw, removing statitics udpate and access
> checks? I think, this won't work. The only concern I see here is that
> process_rw returns a tuple, which is useless for SQL so far.

Ok, I have returned these two functions (as it happens in process_rw).
But I thought that in SQL we would have more “integrated” system
of privilege checks and separate (from box) metrics:

+	rmean_collect(rmean_box, request->type, 1);
+	if (access_check_space(space, PRIV_W) != 0)
+		return -1;

> 
>> @@ -670,30 +689,31 @@ int tarantoolSqlite3ClearTable(int iTable)
>> 			}
>> 		}
>> -		box_iterator_free(iter);
>> -	} else if (box_truncate(space_id) != 0) {
>> +		iterator_delete(iter);
>> +	} else if (box_truncate(space->def->id) != 0) {
> I think we should block box_truncate sp far: too much complications this
> DDL operation introduce. We'll re-implement it as optimization in furture.
> Also, we'll support ANSI SQL's TRUNCATE TABLE.
> 

Fixed on branch. Also, updated comments for OP_Clear.


[-- Attachment #2: Type: text/html, Size: 15087 bytes --]

[tarantool-patches] Re: [PATCH 4/4] sql: replace pgnoRoot with struct space in BtCursor

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 1813 bytes --]


> On 20 Mar 2018, at 13:58, Kirill Yukhin <kyukhin@tarantool.org> wrote:
> 
> Hello,
> 
> Single comment.
> 
> On 19 мар 21:10, Nikita Pettik wrote:
>> Instead of passing encoded space id and index id to SQL bindings,
>> pointers to space and index are saved in cursor and passed implicitly.
>> Space and index lookups appear during execution of
>> OP_OpenRead/OP_OpenWrite once. Moreover, having struct space it has
>> become possible to remove several wrapper-function calls on insertions
>> and deletions by invoking sql_execute_dml().
>> 
>> Closes #3122
>> ---
>> src/box/sql.c              | 87 +++++++++++++++++++++++-----------------------
>> src/box/sql/cursor.h       |  3 +-
>> src/box/sql/tarantoolInt.h |  2 +-
>> src/box/sql/vdbe.c         | 24 ++++++-------
>> src/box/sql/vdbeInt.h      |  1 -
>> 5 files changed, 58 insertions(+), 59 deletions(-)
>> 
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index cd3321c4f..1ee8c4d7b 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -3209,16 +3214,17 @@ case OP_OpenWrite:
>> 	pCur = allocateCursor(p, pOp->p1, nField, CURTYPE_TARANTOOL);
>> 	if (pCur==0) goto no_mem;
>> 	pCur->nullRow = 1;
>> -	pCur->pgnoRoot = p2;
>> 
>> 	assert(p2 >= 1);
>> 	pBtCur = pCur->uc.pCursor;
>> -	pBtCur->pgnoRoot = p2;
>> +	pBtCur->space = space_by_id(SQLITE_PAGENO_TO_SPACEID(p2));
>> +	pBtCur->index = space_index(pBtCur->space, SQLITE_PAGENO_TO_INDEXID(p2));
>> +	assert(pBtCur->space != NULL && pBtCur->index != NULL);
>> 	pBtCur->eState = CURSOR_INVALID;
>> 	pBtCur->curFlags |= BTCF_TaCursor;
>> 	pCur->pKeyInfo = pKeyInfo;
>> 
>> -	open_cursor_set_hints:
>> +//	open_cursor_set_hints:
> Please, don't usse C++ style comments.

Fixed on branch (it is obviously tragic accident).


[-- Attachment #2: Type: text/html, Size: 7125 bytes --]