* [tarantool-patches] [PATCH v2] sql: fix ephemeral table next rowid generation
@ 2018-07-18 15:47 AKhatskevich
2018-07-25 15:06 ` [tarantool-patches] " n.pettik
0 siblings, 1 reply; 3+ messages in thread
From: AKhatskevich @ 2018-07-18 15:47 UTC (permalink / raw)
To: korablev, tarantool-patches
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* [tarantool-patches] Re: [PATCH v2] sql: fix ephemeral table next rowid generation
2018-07-18 15:47 [tarantool-patches] [PATCH v2] sql: fix ephemeral table next rowid generation AKhatskevich
@ 2018-07-25 15:06 ` n.pettik
2018-08-01 17:23 ` Alex Khatskevich
0 siblings, 1 reply; 3+ messages in thread
From: n.pettik @ 2018-07-25 15:06 UTC (permalink / raw)
To: tarantool-patches; +Cc: AKhatskevich
> 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
In fact, to the end of tuple to be inserted. Now ofc PK of ephemeral
spaces consists of all fields, but it may change someday, I guess.
Moreover, ephemeral spaces aren’t supposed to be feature exclusively
of memtx engine. Vinyl ephemeral spaces also have right to exist.
> + * 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)
Firstly, we don’t use ’tarantool_’ prefix anymore. Also we use ’struct’ prefix
for compound types: struct BtCursor *bt_cur. Moreover, I don’t see comment
for this function.
Secondly, I see your message within issue discussion:
"
Discussed verbally with
@kyukhin , @Korablev77 , @kostja and decided, that the unique rowid should be encapsulated into
vtab method of ephemeral space.
“
But this function doesn’t seem to be method of vtab…
> +{
> + 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);
> }
>
>
> 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.
Nitpicking: generally speaking, ephemeral spaces can contain duplicates
when it comes for NULLs. You may investigate how this request works:
SELECT (SELECT NULL UNION SELECT NULL) EXCEPT SELECT NULL;
:)
> */
> 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);
Should we add some checks on rowid overflow? Ephemeral spaces can be extensively
used for JOIN’s where number of inserted to ephemeral space tuples is O(n^2) in
worst case (for two tables). On the other hand, even simple check can slow down execution
especially when it concerns millions of tuples...
> 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);
Use upper-case for all SQL statements pls.
^ permalink raw reply [flat|nested] 3+ messages in thread
* [tarantool-patches] Re: [PATCH v2] sql: fix ephemeral table next rowid generation
2018-07-25 15:06 ` [tarantool-patches] " n.pettik
@ 2018-08-01 17:23 ` Alex Khatskevich
0 siblings, 0 replies; 3+ messages in thread
From: Alex Khatskevich @ 2018-08-01 17:23 UTC (permalink / raw)
To: n.pettik, tarantool-patches
>> 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
> In fact, to the end of tuple to be inserted. Now ofc PK of ephemeral
> spaces consists of all fields, but it may change someday, I guess.
>
> Moreover, ephemeral spaces aren’t supposed to be feature exclusively
> of memtx engine. Vinyl ephemeral spaces also have right to exist.
1. Deleted those details.
2. Created vtab entity `ephemeral_next_rowid`.
>> + * 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)
> Firstly, we don’t use ’tarantool_’ prefix anymore. Also we use ’struct’ prefix
> for compound types: struct BtCursor *bt_cur. Moreover, I don’t see comment
> for this function.
fixed.
Added comment to `tarantoolInt.h`.
> Secondly, I see your message within issue discussion:
> "
> Discussed verbally with
> @kyukhin , @Korablev77 , @kostja and decided, that the unique rowid should be encapsulated into
> vtab method of ephemeral space.
> “
>
> But this function doesn’t seem to be method of vtab…
Created a dedicated vtab function.
>> +{
>> + 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);
>> }
>>
>>
>> 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.
> Nitpicking: generally speaking, ephemeral spaces can contain duplicates
> when it comes for NULLs. You may investigate how this request works:
>
> SELECT (SELECT NULL UNION SELECT NULL) EXCEPT SELECT NULL;
>
> :)
>
>> */
>> 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);
> Should we add some checks on rowid overflow? Ephemeral spaces can be extensively
> used for JOIN’s where number of inserted to ephemeral space tuples is O(n^2) in
> worst case (for two tables). On the other hand, even simple check can slow down execution
> especially when it concerns millions of tuples...
In rowid is increasing 10^9 times per second, it would take 1017 years
to overflow the counter.
So, no.
>> 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);
> Use upper-case for all SQL statements pls.
Fixed.
Full diff
commit 3b30cd4ee27ccf9046a4623d6455479bf980cd1a
Author: AKhatskevich <avkhatskevich@tarantool.org>
Date: Tue May 15 22:00:28 2018 +0300
sql: fix ephemeral table next rowid generation
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
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index f2d512f21..9015c56ca 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -603,6 +603,20 @@ memtx_space_ephemeral_delete(struct space *space,
const char *key)
return 0;
}
+/**
+ * Generate unique number.
+ * This rowid may be used to store non-unique values in an
+ * ephemeral space.
+ *
+ * This function isn't involved in any transaction routine.
+ */
+static uint64_t
+memtx_space_ephemeral_next_rowid(struct space *space)
+{
+ struct memtx_space *memtx_space = (struct memtx_space *)space;
+ return memtx_space->next_rowid++;
+}
+
/* }}} DML */
/* {{{ DDL */
@@ -843,6 +857,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
@@ -949,6 +965,7 @@ static const struct space_vtab memtx_space_vtab = {
/* .execute_upsert = */ memtx_space_execute_upsert,
/* .ephemeral_replace = */ memtx_space_ephemeral_replace,
/* .ephemeral_delete = */ memtx_space_ephemeral_delete,
+ /* .ephemeral_next_rowid = */ memtx_space_ephemeral_next_rowid,
/* .ephemeral_cleanup = */ memtx_space_ephemeral_cleanup,
/* .init_system_space = */ memtx_init_system_space,
/* .init_ephemeral_space = */ memtx_init_ephemeral_space,
diff --git a/src/box/memtx_space.h b/src/box/memtx_space.h
index 7dc341079..bb1d8b74b 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 used to allow to store
+ * non-unique rows in ephemeral tables.
+ */
+ uint64_t next_rowid;
};
/**
diff --git a/src/box/space.h b/src/box/space.h
index 8f675dfe1..e4ae29747 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -70,6 +70,10 @@ struct space_vtab {
int (*ephemeral_replace)(struct space *, const char *, const char *);
int (*ephemeral_delete)(struct space *, const char *);
+ /**
+ * Generate unique number.
+ */
+ uint64_t (*ephemeral_next_rowid)(struct space *);
void (*ephemeral_cleanup)(struct space *);
diff --git a/src/box/sql.c b/src/box/sql.c
index 6104a6d0f..c27595afd 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,15 @@ int tarantoolSqlite3EphemeralDrop(BtCursor *pCur)
return SQLITE_OK;
}
+uint64_t
+ephemeral_next_rowid(struct BtCursor *bt_cur)
+{
+ assert(bt_cur);
+ assert(bt_cur->curFlags & BTCF_TEphemCursor);
+ struct space * ephem_space = (struct space *) bt_cur->space;
+ return ephem_space->vtab->ephemeral_next_rowid(ephem_space);
+}
+
static inline int
insertOrReplace(struct space *space, const char *tuple, const char
*tuple_end,
enum iproto_type type)
@@ -1080,7 +1090,6 @@ key_alloc(BtCursor *cur, size_t key_size)
}
cur->key = new_key;
}
- cur->nKey = key_size;
return 0;
}
@@ -1621,37 +1630,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..38f6a56c1 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -116,8 +116,14 @@ 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);
+/**
+ * Get next rowid for a cursor which points to an ephemeral table.
+ * @param bt_cur Cursor which will point to the new ephemeral space.
+ *
+ * @retval New unique rowid.
+ */
+uint64_t
+ephemeral_next_rowid(struct 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..6ac12dec0 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 = ephemeral_next_rowid(bt_cur);
pOut->flags = MEM_Int;
break;
}
diff --git a/src/box/sysview_engine.c b/src/box/sysview_engine.c
index 308a3328a..64fa6ee23 100644
--- a/src/box/sysview_engine.c
+++ b/src/box/sysview_engine.c
@@ -119,6 +119,14 @@ sysview_space_ephemeral_delete(struct space *space,
const char *key)
return -1;
}
+static uint64_t
+sysview_space_ephemeral_next_rowid(struct space *space)
+{
+ (void)space;
+ unreachable();
+ return 0;
+}
+
static void
sysview_space_ephemeral_cleanup(struct space *space)
{
@@ -206,6 +214,7 @@ static const struct space_vtab sysview_space_vtab = {
/* .execute_upsert = */ sysview_space_execute_upsert,
/* .ephemeral_replace = */ sysview_space_ephemeral_replace,
/* .ephemeral_delete = */ sysview_space_ephemeral_delete,
+ /* .ephemeral_next_rowid = */ sysview_space_ephemeral_next_rowid,
/* .ephemeral_cleanup = */ sysview_space_ephemeral_cleanup,
/* .init_system_space = */ sysview_init_system_space,
/* .init_ephemeral_space = */ sysview_init_ephemeral_space,
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index b7238ae4c..b13b21dfb 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -2379,6 +2379,14 @@ vinyl_space_ephemeral_delete(struct space *space,
const char *key)
return -1;
}
+static uint64_t
+vinyl_space_ephemeral_next_rowid(struct space *space)
+{
+ (void)space;
+ unreachable();
+ return 0;
+}
+
static void
vinyl_space_ephemeral_cleanup(struct space *space)
{
@@ -4053,6 +4061,7 @@ static const struct space_vtab vinyl_space_vtab = {
/* .execute_upsert = */ vinyl_space_execute_upsert,
/* .ephemeral_replace = */ vinyl_space_ephemeral_replace,
/* .ephemeral_delete = */ vinyl_space_ephemeral_delete,
+ /* .ephemeral_next_rowid = */ vinyl_space_ephemeral_next_rowid,
/* .ephemeral_cleanup = */ vinyl_space_ephemeral_cleanup,
/* .init_system_space = */ vinyl_init_system_space,
/* .init_ephemeral_space = */ vinyl_init_ephemeral_space,
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..d49baf300
--- /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()
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-08-01 17:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-18 15:47 [tarantool-patches] [PATCH v2] sql: fix ephemeral table next rowid generation AKhatskevich
2018-07-25 15:06 ` [tarantool-patches] " n.pettik
2018-08-01 17:23 ` Alex Khatskevich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox