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 D5ABB28B48 for ; Thu, 31 May 2018 19:53:50 -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 yk7I1elKDo-M for ; Thu, 31 May 2018 19:53:50 -0400 (EDT) Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 F0AA824E05 for ; Thu, 31 May 2018 19:53:49 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH] Fix ephemeral table next rowid generation From: "n.pettik" In-Reply-To: <20180515194943.5137-1-avkhatskevich@tarantool.org> Date: Fri, 1 Jun 2018 02:53:42 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <5410C9D6-E214-4341-8DD5-4D9986372548@tarantool.org> References: <20180515194943.5137-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: tarantool-patches@freelists.org Cc: AKhatskevich Lets follow SOP conventions: put link to the branch and link to the = issue after =E2=80=98=E2=80=94=E2=80=98. Moreover, add =E2=80=98sql=E2=80=99 prefix to commit subject. As for patch itself, I don=E2=80=99t 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=E2=80=A6Well, we know that rowid is always in the last field, so why just don=E2=80=99t fetch value from last field = and increment it? > On 15 May 2018, at 22:49, AKhatskevich = wrote: >=20 > 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 >=20 > ------ commit message ------------ >=20 > 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. >=20 > 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 >=20 > 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 =3D ephemer_new_space; > pCur->index =3D *ephemer_new_space->index; > + pCur->ephemeral_rowid =3D 0; >=20 > int unused; > return tarantoolSqlite3First(pCur, &unused); > @@ -1080,7 +1081,6 @@ key_alloc(BtCursor *cur, size_t key_size) > } > cur->key =3D new_key; > } > - cur->nKey =3D key_size; > return 0; > } >=20 > @@ -1621,37 +1621,6 @@ sql_debug_info(struct info_handler *h) > info_end(h); > } >=20 > -/* > - * 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 =3D pCur->space; > - assert(ephem_space); > - struct index *primary_index =3D *ephem_space->index; > - > - struct tuple *tuple; > - if (index_max(primary_index, NULL, 0, &tuple) !=3D 0) { > - return SQL_TARANTOOL_ERROR; > - } > - if (tuple =3D=3D NULL) { > - *max_id =3D 0; > - return SQLITE_OK; > - } > - if (tuple_field_u64(tuple, fieldno, max_id) =3D=3D -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 =3D 0; >=20 > srcTab =3D pParse->nTab++; > regRec =3D sqlite3GetTempReg(pParse); > - regCopy =3D sqlite3GetTempRange(pParse, = nColumn); > - regTempId =3D sqlite3GetTempReg(pParse); > + regCopy =3D sqlite3GetTempRange(pParse, nColumn = + 1); > struct key_def *def =3D key_def_new(nColumn + = 1); > if (def =3D=3D 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 =3D 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 =3D = sqlite3GetTempReg(pParse); > /* Last column is required for ID. */ > int regCopy =3D = 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 =3D 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 =3D sqlite3GetTempReg(parse); > int regCopy =3D 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); >=20 > /** > * 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; > } >=20 > -/* Opcode: NextIdEphemeral P1 P2 P3 * * > - * Synopsis: r[P3]=3Dget_max(space_index[P1]{Column[P2]}) > +/* Opcode: NextIdEphemeral P1 P2 * * * > + * Synopsis: r[P2]=3Dget_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 =3D p->apCsr[pOp->p1]; > - p2 =3D pOp->p2; > - pOut =3D &aMem[pOp->p3]; > - > - assert(pC->uc.pCursor->curFlags & BTCF_TEphemCursor); > - > - rc =3D tarantoolSqlite3EphemeralGetMaxId(pC->uc.pCursor, p2, > - (uint64_t *) &pOut->u.i); > - if (rc) goto abort_due_to_error; > - > - pOut->u.i +=3D 1; > + pOut =3D &aMem[pOp->p2]; > + struct BtCursor * bt_cur =3D pC->uc.pCursor; > + assert(bt_cur->curFlags & BTCF_TEphemCursor); > + pOut->u.i =3D bt_cur->ephemeral_rowid++; > pOut->flags =3D 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 =3D 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() > --=20 > 2.14.1 >=20 >=20