[tarantool-patches] [PATCH 2/2] sql: use vtab::rowid_next() instead of index_max()

[Nikita Pettik <korablev@tarantool.org>]

After introducing separate method in space's vtab to fetch next rowid
value, lets use it in SQL internals. This allows us to fix incorrect
results of queries involving storing equal tuples in ephemeral spaces.

Closes #3297
---
 src/box/sql.c                                 | 28 +------------------
 src/box/sql/insert.c                          | 17 +++---------
 src/box/sql/select.c                          | 12 ++++-----
 src/box/sql/tarantoolInt.h                    | 13 ---------
 src/box/sql/vdbe.c                            | 39 +++++++++++++++++----------
 test/sql-tap/gh-3297-ephemeral-rowid.test.lua | 30 +++++++++++++++++++++
 test/sql/errinj.result                        | 15 +++++++++++
 test/sql/errinj.test.lua                      |  7 +++++
 8 files changed, 87 insertions(+), 74 deletions(-)
 create mode 100755 test/sql-tap/gh-3297-ephemeral-rowid.test.lua

diff --git a/src/box/sql.c b/src/box/sql.c
index c7b87e57a..c4fc4b49c 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -41,6 +41,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"
@@ -1434,33 +1435,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 space Pointer 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(struct space *space, uint32_t fieldno,
-				      uint64_t *max_id)
-{
-	struct index *primary_index = *space->index;
-	struct tuple *tuple;
-	if (index_max(primary_index, NULL, 0, &tuple) != 0)
-		return -1;
-	if (tuple == NULL) {
-		*max_id = 0;
-		return 0;
-	}
-	if (tuple_field_u64(tuple, fieldno, max_id) == -1)
-		return -1;
-
-	return 0;
-}
-
 int
 tarantoolSqlNextSeqId(uint64_t *max_id)
 {
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 5862917f0..fc8b007bc 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -461,29 +461,19 @@ 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++;
 			reg_eph = ++pParse->nMem;
 			regRec = sqlite3GetTempReg(pParse);
-			regCopy = sqlite3GetTempRange(pParse, nColumn);
-			regTempId = sqlite3GetTempReg(pParse);
+			regCopy = sqlite3GetTempRange(pParse, nColumn + 1);
 			sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
 					  nColumn + 1);
-
-			/* 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, reg_eph,
+					  regCopy + nColumn);
 			sqlite3VdbeAddOp3(v, OP_Copy, regFromSelect, regCopy, nColumn-1);
 			sqlite3VdbeAddOp3(v, OP_MakeRecord, regCopy,
 					  nColumn + 1, regRec);
@@ -494,7 +484,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 77e0c5d66..b191f318b 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1154,8 +1154,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, pDest->reg_eph,
-						  nResultCol, regCopy + nResultCol);
+				sqlite3VdbeAddOp2(v, OP_NextIdEphemeral, pDest->reg_eph,
+						  regCopy + nResultCol);
 				/* Positioning ID column to be last in inserted tuple.
 				 * NextId -> regCopy + n + 1
 				 * Copy [regResult, regResult + n] -> [regCopy, regCopy + n]
@@ -1601,8 +1601,8 @@ generateSortTail(Parse * pParse,	/* Parsing context */
 	case SRT_Table:
 	case SRT_EphemTab: {
 			int regCopy = sqlite3GetTempRange(pParse,  nColumn);
-			sqlite3VdbeAddOp3(v, OP_NextIdEphemeral, pDest->reg_eph,
-					  nColumn, regTupleid);
+			sqlite3VdbeAddOp2(v, OP_NextIdEphemeral, pDest->reg_eph,
+					  regTupleid);
 			sqlite3VdbeAddOp3(v, OP_Copy, regRow, regCopy, nSortData - 1);
 			sqlite3VdbeAddOp3(v, OP_MakeRecord, regCopy, nColumn + 1, regRow);
 			sqlite3VdbeAddOp2(v, OP_IdxInsert, regRow, pDest->reg_eph);
@@ -3127,8 +3127,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->reg_eph,
-					  in->nSdst, regCopy + in->nSdst);
+			sqlite3VdbeAddOp2(v, OP_NextIdEphemeral, dest->reg_eph,
+					  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 daab3c84b..7d63b6fd6 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -130,19 +130,6 @@ int tarantoolSqlite3EphemeralCount(BtCursor * pCur, i64 * pnEntry);
 int tarantoolSqlite3EphemeralDrop(BtCursor * pCur);
 int tarantoolSqlite3EphemeralClearTable(BtCursor * pCur);
 
-/**
- * Extract maximum integer value from ephemeral space.
- * If index is empty - return 0 in max_id and success status.
- *
- * @param space Pointer 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(struct space *space, uint32_t fieldno,
-				       uint64_t * max_id);
-
 /**
  * Performs exactly as extract_key + sqlite3VdbeCompareMsgpack,
  * only faster.
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7c1015cf9..c730c70cb 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3685,25 +3685,36 @@ 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[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 uses register P1 to
- * fetch pointer to epehemeral space.
+ * This opcode stores next `rowid` for the ephemeral space to
+ * P2 register. `rowid` is required, because inserted to
+ * ephemeral space tuples may be not unique. Meanwhile,
+ * Tarantool`s ephemeral spaces can contain only unique tuples
+ * due to only one index (which is PK over all columns in space).
  */
 case OP_NextIdEphemeral: {
 	struct space *space = (struct space*)p->aMem[pOp->p1].u.p;
-	int p2 = pOp->p2;
-	assert(space != NULL);
-	pOut = &aMem[pOp->p3];
-	rc = tarantoolSqlite3EphemeralGetMaxId(space, p2,
-					       (uint64_t *) &pOut->u.i);
-	if (rc != 0)
+	assert(space->def->id == 0);
+	uint64_t rowid;
+	if (space->vtab->ephemeral_rowid_next(space, &rowid) != 0) {
+		rc = SQL_TARANTOOL_ERROR;
 		goto abort_due_to_error;
-	pOut->u.i += 1;
+	}
+	/*
+	 * FIXME: since memory cell can comprise only 32-bit
+	 * integer, make sure it can fit in. This check should
+	 * be removed when memory cell is extended with unsigned
+	 * 64-bit integer.
+	 */
+	if (rowid > INT32_MAX) {
+		diag_set(ClientError, ER_ROWID_OVERFLOW);
+		rc = SQL_TARANTOOL_ERROR;
+		goto abort_due_to_error;
+	}
+	pOut = &aMem[pOp->p2];
+	pOut->u.i = rowid;
 	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..dd2b07cc5
--- /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 INT PRIMARY KEY);
+    CREATE TABLE T2(A INT PRIMARY KEY, B INT);
+    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()
diff --git a/test/sql/errinj.result b/test/sql/errinj.result
index a0ba60f45..60f776c3c 100644
--- a/test/sql/errinj.result
+++ b/test/sql/errinj.result
@@ -279,3 +279,18 @@ errinj.set("ERRINJ_WAL_IO", false)
 box.sql.execute("DROP TABLE t3;")
 ---
 ...
+-- Make sure that overflow of rowid used for ephemeral spaces
+-- is hadnled properly.
+--
+box.error.injection.set("ERRINJ_ROWID_OVERFLOW", true)
+---
+- ok
+...
+box.sql.execute("WITH RECURSIVE tmp AS (SELECT 1 UNION ALL SELECT * FROM tmp LIMIT 2) SELECT * FROM tmp;")
+---
+- error: 'Rowid is overflowed: too many entries in ephemeral space'
+...
+box.error.injection.set("ERRINJ_ROWID_OVERFLOW", false)
+---
+- ok
+...
diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua
index 25d73f0c2..034a43d4e 100644
--- a/test/sql/errinj.test.lua
+++ b/test/sql/errinj.test.lua
@@ -97,3 +97,10 @@ box.sql.execute("ALTER TABLE t3 DROP CONSTRAINT fk1;")
 box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);")
 errinj.set("ERRINJ_WAL_IO", false)
 box.sql.execute("DROP TABLE t3;")
+
+-- Make sure that overflow of rowid used for ephemeral spaces
+-- is hadnled properly.
+--
+box.error.injection.set("ERRINJ_ROWID_OVERFLOW", true)
+box.sql.execute("WITH RECURSIVE tmp AS (SELECT 1 UNION ALL SELECT * FROM tmp LIMIT 2) SELECT * FROM tmp;")
+box.error.injection.set("ERRINJ_ROWID_OVERFLOW", false)
-- 
2.15.1