[tarantool-patches] [PATCH v2] sql: fix ephemeral table next rowid generation

[AKhatskevich <avkhatskevich@tarantool.org>]

Before this commit, next rowid was retrieved from an ephemeral table
using `index_max`. That led to wrong behavior because the index is not
sorted by rowid and `index_max` doesn`t return a tuple with the greatest
rowid.

New implementation stores next `rowid` value in `struct memtx_space`,
and increments it after each insert to the ephemeral space.

Extra refactoring:
 * `nKey` attribute is deleted from BtCursor, because it is not used
anymore.

Closes #3297
---
Changes in v2:
* ephemeral id is stored in memtx_space object

Branch: https://github.com/tarantool/tarantool/tree/kh/gh-3297-fix_ephem_id-2
Issue: https://github.com/tarantool/tarantool/issues/3297

 src/box/memtx_space.c                         |  2 ++
 src/box/memtx_space.h                         |  5 +++
 src/box/sql.c                                 | 45 ++++++++-------------------
 src/box/sql/cursor.h                          |  1 -
 src/box/sql/insert.c                          | 16 ++--------
 src/box/sql/select.c                          | 10 +++---
 src/box/sql/tarantoolInt.h                    |  4 +--
 src/box/sql/vdbe.c                            | 24 +++++---------
 test/sql-tap/gh-3297_ephemeral_rowid.test.lua | 30 ++++++++++++++++++
 9 files changed, 68 insertions(+), 69 deletions(-)
 create mode 100755 test/sql-tap/gh-3297_ephemeral_rowid.test.lua

diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index f2d512f21..5a80d48ab 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -843,6 +843,8 @@ static void
 memtx_init_ephemeral_space(struct space *space)
 {
 	memtx_space_add_primary_key(space);
+	struct memtx_space * ephem_space = (struct memtx_space *) space;
+	ephem_space->next_rowid = 0;
 }
 
 static int
diff --git a/src/box/memtx_space.h b/src/box/memtx_space.h
index 7dc341079..26181166a 100644
--- a/src/box/memtx_space.h
+++ b/src/box/memtx_space.h
@@ -48,6 +48,11 @@ struct memtx_space {
 	 */
 	int (*replace)(struct space *, struct tuple *, struct tuple *,
 		       enum dup_replace_mode, struct tuple **);
+	/**
+	 * Next rowid. This number is added to the end of pk to
+	 * allow to store non-unique rows in ephemeral tables.
+	 */
+	uint64_t next_rowid;
 };
 
 /**
diff --git a/src/box/sql.c b/src/box/sql.c
index 6104a6d0f..fdf0313d3 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -47,6 +47,7 @@
 #include "box.h"
 #include "txn.h"
 #include "space.h"
+#include "memtx_space.h"
 #include "space_def.h"
 #include "index_def.h"
 #include "tuple.h"
@@ -448,6 +449,18 @@ int tarantoolSqlite3EphemeralDrop(BtCursor *pCur)
 	return SQLITE_OK;
 }
 
+/**
+ * Get next rowid for an ephemeral table.
+ */
+uint64_t
+tarantool_ephemeral_next_rowid(BtCursor *bt_cur)
+{
+	assert(bt_cur);
+	assert(bt_cur->curFlags & BTCF_TEphemCursor);
+	struct memtx_space * ephem_space = (struct memtx_space *) bt_cur->space;
+	return ephem_space->next_rowid++;
+}
+
 static inline int
 insertOrReplace(struct space *space, const char *tuple, const char *tuple_end,
 		enum iproto_type type)
@@ -1080,7 +1093,6 @@ key_alloc(BtCursor *cur, size_t key_size)
 		}
 		cur->key = new_key;
 	}
-	cur->nKey = key_size;
 	return 0;
 }
 
@@ -1621,37 +1633,6 @@ sql_debug_info(struct info_handler *h)
 	info_end(h);
 }
 
-/*
- * Extract maximum integer value from ephemeral space.
- * If index is empty - return 0 in max_id and success status.
- *
- * @param pCur Cursor pointing to ephemeral space.
- * @param fieldno Number of field from fetching tuple.
- * @param[out] max_id Fetched max value.
- *
- * @retval 0 on success, -1 otherwise.
- */
-int tarantoolSqlite3EphemeralGetMaxId(BtCursor *pCur, uint32_t fieldno,
-				       uint64_t *max_id)
-{
-	struct space *ephem_space = pCur->space;
-	assert(ephem_space);
-	struct index *primary_index = *ephem_space->index;
-
-	struct tuple *tuple;
-	if (index_max(primary_index, NULL, 0, &tuple) != 0) {
-		return SQL_TARANTOOL_ERROR;
-	}
-	if (tuple == NULL) {
-		*max_id = 0;
-		return SQLITE_OK;
-	}
-	if (tuple_field_u64(tuple, fieldno, max_id) == -1)
-		return SQL_TARANTOOL_ERROR;
-
-	return SQLITE_OK;
-}
-
 int
 tarantoolSqlNextSeqId(uint64_t *max_id)
 {
diff --git a/src/box/sql/cursor.h b/src/box/sql/cursor.h
index c55941784..ab379f01e 100644
--- a/src/box/sql/cursor.h
+++ b/src/box/sql/cursor.h
@@ -57,7 +57,6 @@ typedef struct BtCursor BtCursor;
  * for ordinary space, or to TEphemCursor for ephemeral space.
  */
 struct BtCursor {
-	i64 nKey;		/* Size of pKey, or last integer key */
 	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() */
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 0b9e75a93..bddfdc50c 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -557,15 +557,12 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 			 *      M: ...
 			 */
 			int regRec;	/* Register to hold packed record */
-			int regTempId;	/* Register to hold temp table ID */
 			int regCopy;    /* Register to keep copy of registers from select */
 			int addrL;	/* Label "L" */
-			int64_t initial_pk = 0;
 
 			srcTab = pParse->nTab++;
 			regRec = sqlite3GetTempReg(pParse);
-			regCopy = sqlite3GetTempRange(pParse, nColumn);
-			regTempId = sqlite3GetTempReg(pParse);
+			regCopy = sqlite3GetTempRange(pParse, nColumn + 1);
 			struct key_def *def = key_def_new(nColumn + 1);
 			if (def == NULL) {
 				sqlite3OomFault(db);
@@ -573,16 +570,10 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 			}
 			sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, srcTab, nColumn+1,
 					  0, (char*)def, P4_KEYDEF);
-			/* Create counter for rowid */
-			sqlite3VdbeAddOp4Dup8(v, OP_Int64,
-					      0 /* unused */,
-					      regTempId,
-					      0 /* unused */,
-					      (const unsigned char*) &initial_pk,
-					      P4_INT64);
 			addrL = sqlite3VdbeAddOp1(v, OP_Yield, dest.iSDParm);
 			VdbeCoverage(v);
-			sqlite3VdbeAddOp2(v, OP_AddImm, regTempId, 1);
+			sqlite3VdbeAddOp2(v, OP_NextIdEphemeral, srcTab,
+					  regCopy + nColumn);
 			sqlite3VdbeAddOp3(v, OP_Copy, regFromSelect, regCopy, nColumn-1);
 			sqlite3VdbeAddOp3(v, OP_MakeRecord, regCopy,
 					  nColumn + 1, regRec);
@@ -593,7 +584,6 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 			sqlite3VdbeGoto(v, addrL);
 			sqlite3VdbeJumpHere(v, addrL);
 			sqlite3ReleaseTempReg(pParse, regRec);
-			sqlite3ReleaseTempReg(pParse, regTempId);
 			sqlite3ReleaseTempRange(pParse, regCopy, nColumn);
 		}
 	} else {
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 1f27efdb5..5cd289ccc 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1047,8 +1047,8 @@ selectInnerLoop(Parse * pParse,		/* The parser context */
 				int regRec = sqlite3GetTempReg(pParse);
 				/* Last column is required for ID. */
 				int regCopy = sqlite3GetTempRange(pParse, nResultCol + 1);
-				sqlite3VdbeAddOp3(v, OP_NextIdEphemeral, iParm,
-						  nResultCol, regCopy + nResultCol);
+				sqlite3VdbeAddOp2(v, OP_NextIdEphemeral, iParm,
+						  regCopy + nResultCol);
 				/* Positioning ID column to be last in inserted tuple.
 				 * NextId -> regCopy + n + 1
 				 * Copy [regResult, regResult + n] -> [regCopy, regCopy + n]
@@ -1418,7 +1418,7 @@ generateSortTail(Parse * pParse,	/* Parsing context */
 	case SRT_Table:
 	case SRT_EphemTab: {
 			int regCopy = sqlite3GetTempRange(pParse,  nColumn);
-			sqlite3VdbeAddOp3(v, OP_NextIdEphemeral, iParm, nColumn, regTupleid);
+			sqlite3VdbeAddOp2(v, OP_NextIdEphemeral, iParm, regTupleid);
 			sqlite3VdbeAddOp3(v, OP_Copy, regRow, regCopy, nSortData - 1);
 			sqlite3VdbeAddOp3(v, OP_MakeRecord, regCopy, nColumn + 1, regRow);
 			sqlite3VdbeAddOp2(v, OP_IdxInsert, iParm, regRow);
@@ -2898,8 +2898,8 @@ generateOutputSubroutine(struct Parse *parse, struct Select *p,
 	case SRT_EphemTab:{
 			int regRec = sqlite3GetTempReg(parse);
 			int regCopy = sqlite3GetTempRange(parse, in->nSdst + 1);
-			sqlite3VdbeAddOp3(v, OP_NextIdEphemeral, dest->iSDParm,
-					  in->nSdst, regCopy + in->nSdst);
+			sqlite3VdbeAddOp2(v, OP_NextIdEphemeral, dest->iSDParm,
+					  regCopy + in->nSdst);
 			sqlite3VdbeAddOp3(v, OP_Copy, in->iSdst, regCopy,
 					  in->nSdst - 1);
 			sqlite3VdbeAddOp3(v, OP_MakeRecord, regCopy,
diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
index 94f94cb44..369af77ff 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -116,8 +116,8 @@ int tarantoolSqlite3EphemeralDelete(BtCursor * pCur);
 int tarantoolSqlite3EphemeralCount(BtCursor * pCur, i64 * pnEntry);
 int tarantoolSqlite3EphemeralDrop(BtCursor * pCur);
 int tarantoolSqlite3EphemeralClearTable(BtCursor * pCur);
-int tarantoolSqlite3EphemeralGetMaxId(BtCursor * pCur, uint32_t fieldno,
-				       uint64_t * max_id);
+uint64_t
+tarantool_ephemeral_next_rowid(BtCursor *bt_cur);
 
 /**
  * Performs exactly as extract_key + sqlite3VdbeCompareMsgpack,
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 89c35d218..532c98e54 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3790,27 +3790,19 @@ case OP_NextSequenceId: {
 	break;
 }
 
-/* Opcode: NextIdEphemeral P1 P2 P3 * *
- * Synopsis: r[P3]=get_max(space_index[P1]{Column[P2]})
+/* Opcode: NextIdEphemeral P1 P2 * * *
+ * Synopsis: r[P2]=get_next_rowid(space_index[P1]})
  *
- * This opcode works in the same way as OP_NextId does, except it is
- * only applied for ephemeral tables. The difference is in the fact that
- * all ephemeral tables don't have space_id (to be more precise it equals to zero).
+ * This opcode stores next `rowid` for the ephemeral space to P2 register.
+ * `rowid` is necessary, because inserted to ephemeral space tuples may be not
+ * unique, while Tarantool`s spaces can contain only unique tuples.
  */
 case OP_NextIdEphemeral: {
 	VdbeCursor *pC;
-	int p2;
 	pC = p->apCsr[pOp->p1];
-	p2 = pOp->p2;
-	pOut = &aMem[pOp->p3];
-
-	assert(pC->uc.pCursor->curFlags & BTCF_TEphemCursor);
-
-	rc = tarantoolSqlite3EphemeralGetMaxId(pC->uc.pCursor, p2,
-					       (uint64_t *) &pOut->u.i);
-	if (rc) goto abort_due_to_error;
-
-	pOut->u.i += 1;
+	pOut = &aMem[pOp->p2];
+	struct BtCursor * bt_cur = pC->uc.pCursor;
+	pOut->u.i = tarantool_ephemeral_next_rowid(bt_cur);
 	pOut->flags = MEM_Int;
 	break;
 }
diff --git a/test/sql-tap/gh-3297_ephemeral_rowid.test.lua b/test/sql-tap/gh-3297_ephemeral_rowid.test.lua
new file mode 100755
index 000000000..ed596e7ad
--- /dev/null
+++ b/test/sql-tap/gh-3297_ephemeral_rowid.test.lua
@@ -0,0 +1,30 @@
+#!/usr/bin/env tarantool
+test = require("sqltester")
+test:plan(1)
+
+-- Check that OP_NextIdEphemeral generates unique ids.
+
+test:execsql [[
+    CREATE TABLE t1(a primary key);
+    CREATE TABLE t2(a primary key, b);
+    insert into t1 values(12);
+    insert into t2 values(1, 5);
+    insert into t2 values(2, 2);
+    insert into t2 values(3, 2);
+    insert into t2 values(4, 2);
+]]
+
+test:do_execsql_test(
+    "gh-3297-1",
+    [[
+        select * from ( select a from t1 limit 1), (select b from t2 limit 10);
+    ]],
+    {
+        12, 2,
+        12, 2,
+        12, 2,
+        12, 5
+    }
+)
+
+test:finish_test()
-- 
2.14.1