[tarantool-patches] [PATCH] Fix ephemeral table next rowid generation

[tarantool-patches] [PATCH] Fix ephemeral table next rowid generation

[AKhatskevich]

branch: kh/gh-3297-fix_ephem_id-2
notes: Kiril asked me to store rowid in BtCursor (implemented in this patch)
       however I prefer to create a single static variable for the whole
       tarantool (rowid just need to be unique in a space) as implemented
       here: kh/gh-3297-fix_ephem_id-1

------ commit message ------------

Next `rowid` value is stored in BtCursor, and is incremented after each
insert to the ephemeral space.

Closes #3297
---
 src/box/sql.c                                 | 33 +--------------------------
 src/box/sql/cursor.h                          |  2 +-
 src/box/sql/insert.c                          | 16 +++----------
 src/box/sql/select.c                          | 10 ++++----
 src/box/sql/tarantoolInt.h                    |  2 --
 src/box/sql/vdbe.c                            | 25 ++++++++------------
 test/sql-tap/gh-3297_ephemeral_rowid.test.lua | 30 ++++++++++++++++++++++++
 7 files changed, 49 insertions(+), 69 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 6104a6d0f..cd7ac8a50 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -423,6 +423,7 @@ int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
 	}
 	pCur->space = ephemer_new_space;
 	pCur->index = *ephemer_new_space->index;
+	pCur->ephemeral_rowid = 0;
 
 	int unused;
 	return tarantoolSqlite3First(pCur, &unused);
@@ -1080,7 +1081,6 @@ key_alloc(BtCursor *cur, size_t key_size)
 		}
 		cur->key = new_key;
 	}
-	cur->nKey = key_size;
 	return 0;
 }
 
@@ -1621,37 +1621,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..6958a9f23 100644
--- a/src/box/sql/cursor.h
+++ b/src/box/sql/cursor.h
@@ -57,7 +57,7 @@ typedef struct BtCursor BtCursor;
  * for ordinary space, or to TEphemCursor for ephemeral space.
  */
 struct BtCursor {
-	i64 nKey;		/* Size of pKey, or last integer key */
+	u64 ephemeral_rowid;	/* Next unique id for Ephemeral space */
 	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..7da1f423b 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -116,8 +116,6 @@ 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);
 
 /**
  * Performs exactly as extract_key + sqlite3VdbeCompareMsgpack,
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 89c35d218..743d6e8c5 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3790,27 +3790,20 @@ 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;
+	assert(bt_cur->curFlags & BTCF_TEphemCursor);
+	pOut->u.i = bt_cur->ephemeral_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..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

[tarantool-patches] Re: [PATCH] Fix ephemeral table next rowid generation

[n.pettik]

Lets follow SOP conventions: put link to the branch and link to the issue after ‘—‘.

Moreover, add ‘sql’ prefix to commit subject.

As for patch itself, I don’t like the idea to store rowid for ephemeral space
in cursor: if insertion in such space occurred not via SQL facilities, then
rowid would be wrong…Well, we know that rowid is always in
the last field, so why just don’t fetch value from last field and increment it?

> On 15 May 2018, at 22:49, AKhatskevich <avkhatskevich@tarantool.org> wrote:
> 
> branch: kh/gh-3297-fix_ephem_id-2
> notes: Kiril asked me to store rowid in BtCursor (implemented in this patch)
>       however I prefer to create a single static variable for the whole
>       tarantool (rowid just need to be unique in a space) as implemented
>       here: kh/gh-3297-fix_ephem_id-1
> 
> ------ commit message ------------
> 
> Next `rowid` value is stored in BtCursor, and is incremented after each
> insert to the ephemeral space.

It would be better if you explained provided changes. Without explanation
it is quite complicated to understand aim of the patch.
Also, in this patch, for instance, you have removed field from BtCursor,
so lets mention this fact.

> 
> Closes #3297
> ---
> src/box/sql.c                                 | 33 +--------------------------
> src/box/sql/cursor.h                          |  2 +-
> src/box/sql/insert.c                          | 16 +++----------
> src/box/sql/select.c                          | 10 ++++----
> src/box/sql/tarantoolInt.h                    |  2 --
> src/box/sql/vdbe.c                            | 25 ++++++++------------
> test/sql-tap/gh-3297_ephemeral_rowid.test.lua | 30 ++++++++++++++++++++++++
> 7 files changed, 49 insertions(+), 69 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 6104a6d0f..cd7ac8a50 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -423,6 +423,7 @@ int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
> 	}
> 	pCur->space = ephemer_new_space;
> 	pCur->index = *ephemer_new_space->index;
> +	pCur->ephemeral_rowid = 0;
> 
> 	int unused;
> 	return tarantoolSqlite3First(pCur, &unused);
> @@ -1080,7 +1081,6 @@ key_alloc(BtCursor *cur, size_t key_size)
> 		}
> 		cur->key = new_key;
> 	}
> -	cur->nKey = key_size;
> 	return 0;
> }
> 
> @@ -1621,37 +1621,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..6958a9f23 100644
> --- a/src/box/sql/cursor.h
> +++ b/src/box/sql/cursor.h
> @@ -57,7 +57,7 @@ typedef struct BtCursor BtCursor;
>  * for ordinary space, or to TEphemCursor for ephemeral space.
>  */
> struct BtCursor {
> -	i64 nKey;		/* Size of pKey, or last integer key */
> +	u64 ephemeral_rowid;	/* Next unique id for Ephemeral space */
> 	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..7da1f423b 100644
> --- a/src/box/sql/tarantoolInt.h
> +++ b/src/box/sql/tarantoolInt.h
> @@ -116,8 +116,6 @@ 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);
> 
> /**
>  * Performs exactly as extract_key + sqlite3VdbeCompareMsgpack,
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 89c35d218..743d6e8c5 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -3790,27 +3790,20 @@ 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;
> +	assert(bt_cur->curFlags & BTCF_TEphemCursor);
> +	pOut->u.i = bt_cur->ephemeral_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..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
> 
> 

[tarantool-patches] Re: [PATCH] Fix ephemeral table next rowid generation

[n.pettik]

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


>> As for patch itself, I don’t like the idea to store rowid for ephemeral space
>> in cursor: if insertion in such space occurred not via SQL facilities, then
>> rowid would be wrong…Well, we know that rowid is always in
>> the last field, so why just don’t fetch value from last field and increment it?
> Rowid cannot be stored in the last field, because tuples are sorted by the first parts.

Wait, where is rowid stored now? AFAIU it is still placed in the last field.

> That means that means that greatest tuple may not has the greatest rowid.

Now I am thinking about introducing secondary index for table with rowid on the last field.
Could you take a look at code and say whether this idea feasible or not?
In this case, it would be easy to fetch max value from ‘last’ field and increment it.
Anyway even if this idea fails as well, lets come up with better solution.

PS add patches mailing list to receivers so that our conversation is available for everyone.


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

[tarantool-patches] Re: [PATCH] Fix ephemeral table next rowid generation

[Alex Khatskevich]

> Wait, where is rowid stored now? AFAIU it is still placed in the last field.
Yes. We still need the rowid to be the latest field, to make tuples unique.
However we cannot get the largest rowid by callind `index_max`. That was 
a point.
> Now I am thinking about introducing secondary index for table with rowid on the last field.
> Could you take a look at code and say whether this idea feasible or not?
I am absolutely against it. It is overkill, to maintain the whole index 
just to implement a
counter.
> In this case, it would be easy to fetch max value from ‘last’ field and increment it.
> Anyway even if this idea fails as well, lets come up with better solution.
Why you do not like saving the counter in the cursor?

There is an option to maintain uint64 global counter, for all temp space 
rowids.
Rowids would not go sequentially anymore (gaps are possible), but we 
would preserve the main constraint: tuples would be unique. In that case 
we do not have to keep extra attribute in each
cursor.
> PS add patches mailing list to receivers so that our conversation is available for everyone.
Ouch. Done.

[tarantool-patches] Re: [PATCH] Fix ephemeral table next rowid generation

[n.pettik]

>> Now I am thinking about introducing secondary index for table with rowid on the last field.
>> Could you take a look at code and say whether this idea feasible or not?
> I am absolutely against it. It is overkill, to maintain the whole index just to implement a
> counter.

Well, I am of the opinion that it is not overkill, but proper solution.
Lets ask smb else for advice. Personally I strictly against your approach.

>> In this case, it would be easy to fetch max value from ‘last’ field and increment it.
>> Anyway even if this idea fails as well, lets come up with better solution.
> Why you do not like saving the counter in the cursor?

Since cursor is a kind of wrapper around iterator, i.e. it represents only interface of 
internal routine. On the other hand, rowid belongs to the ‘user’ space. 
Moreover, not all ephemeral spaces needs this rowid. Finally, if smith went wrong
and insertion in such space occurred not via this cursor, then rowid would be
Incorrect and no warning or error would appear. I would like to remind you that
we wanted to avoid using cursors for insertions and removals.

> There is an option to maintain uint64 global counter, for all temp space rowids.

Hm, once I heard that using global variables might lead you straight to the hell...