From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 147D027408 for ; Wed, 18 Jul 2018 11:47:40 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id swdAteBDA1cg for ; Wed, 18 Jul 2018 11:47:39 -0400 (EDT) Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 93AFF220A8 for ; Wed, 18 Jul 2018 11:47:39 -0400 (EDT) From: AKhatskevich Subject: [tarantool-patches] [PATCH v2] sql: fix ephemeral table next rowid generation Date: Wed, 18 Jul 2018 18:47:18 +0300 Message-Id: <20180718154718.3103-1-avkhatskevich@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: korablev@tarantool.org, tarantool-patches@freelists.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