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 A1B0123B61 for ; Wed, 25 Jul 2018 11:06:27 -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 akhGqgEe6YhW for ; Wed, 25 Jul 2018 11:06:27 -0400 (EDT) Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (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 0A91720090 for ; Wed, 25 Jul 2018 11:06:26 -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 v2] sql: fix ephemeral table next rowid generation From: "n.pettik" In-Reply-To: <20180718154718.3103-1-avkhatskevich@tarantool.org> Date: Wed, 25 Jul 2018 18:06:22 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <1368F28D-35EB-4BFA-B672-2BD8BD37963D@tarantool.org> References: <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: tarantool-patches@freelists.org 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 =3D (struct memtx_space *) = space; > + ephem_space->next_rowid =3D 0; > } >=20 > 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=E2=80=99t supposed to be feature = exclusively of memtx engine. Vinyl ephemeral spaces also have right to exist.=20 > + * allow to store non-unique rows in ephemeral tables. > + */ > + uint64_t next_rowid; > }; >=20 > /** > 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; > } >=20 > +/** > + * Get next rowid for an ephemeral table. > + */ > +uint64_t > +tarantool_ephemeral_next_rowid(BtCursor *bt_cur) Firstly, we don=E2=80=99t use =E2=80=99tarantool_=E2=80=99 prefix = anymore. Also we use =E2=80=99struct=E2=80=99 prefix for compound types: struct BtCursor *bt_cur. Moreover, I don=E2=80=99t = 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. =E2=80=9C But this function doesn=E2=80=99t seem to be method of vtab=E2=80=A6 > +{ > + assert(bt_cur); > + assert(bt_cur->curFlags & BTCF_TEphemCursor); > + struct memtx_space * ephem_space =3D (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 =3D new_key; > } > - cur->nKey =3D key_size; > return 0; > } >=20 > @@ -1621,37 +1633,6 @@ sql_debug_info(struct info_handler *h) > info_end(h); > } >=20 >=20 > 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); >=20 > /** > * 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; > } >=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. 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 =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; > + pOut->u.i =3D tarantool_ephemeral_next_rowid(bt_cur); Should we add some checks on rowid overflow? Ephemeral spaces can be = extensively used for JOIN=E2=80=99s 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 =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); Use upper-case for all SQL statements pls.