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
