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 A2E7E2FDE3 for ; Fri, 9 Nov 2018 04:25:07 -0500 (EST) 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 uaeNlNh1mfV8 for ; Fri, 9 Nov 2018 04:25:07 -0500 (EST) Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (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 615D62FDE2 for ; Fri, 9 Nov 2018 04:25:07 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 1/2] space: add method to fetch next rowid References: <11f65a415a9b1101fa4ba816be237df524de9b47.1540838910.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <52d0faf6-598d-c29b-9b9d-50f4826573eb@tarantool.org> Date: Fri, 9 Nov 2018 12:25:03 +0300 MIME-Version: 1.0 In-Reply-To: <11f65a415a9b1101fa4ba816be237df524de9b47.1540838910.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Nikita Pettik , tarantool-patches@freelists.org Hi! Thanks for the patch! I understand, that Vova said that it should not be pushed, but Kirill asked me, on the contrary, to review it. So I do. On 29/10/2018 22:02, Nikita Pettik wrote: > Ephemeral space are extensively used in SQL to store intermediate > results of query processing. To keep things simple, they feature only > one unique index (primary) which covers all fields. However, ephemeral > space can be used to store non-unique entries. In this case, one > additional field added to the end if stored data: > > [field1, ... fieldn, rowid] > > Note that it can't be added to the beginning of tuple since data in > ephemeral space may be kept as sorted. Previously, to generate proper > rowid index_max() was used. However, it is obviously wrong way to do it. > Hence, lets add simple integer counter to memtx space (ephemeral spaces > are valid only for memtx engine) and introduce method in vtab to fetch > next rowid value. > > Needed for #3297 > --- > src/box/blackhole.c | 1 + > src/box/errcode.h | 2 ++ > src/box/memtx_space.c | 17 +++++++++++++++++ > src/box/memtx_space.h | 7 +++++++ > src/box/space.c | 9 +++++++++ > src/box/space.h | 3 +++ > src/box/sysview.c | 1 + > src/box/vinyl.c | 1 + > src/errinj.h | 1 + > test/box/errinj.result | 2 ++ > test/box/misc.result | 1 + > 11 files changed, 45 insertions(+) > > index 04f4f34ee..fab8b6617 100644 > --- a/src/box/errcode.h > +++ b/src/box/errcode.h > @@ -223,6 +223,8 @@ struct errcode_record { > /*168 */_(ER_DROP_FK_CONSTRAINT, "Failed to drop foreign key constraint '%s': %s") \ > /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint %s does not exist") \ > /*170 */_(ER_CONSTRAINT_EXISTS, "Constraint %s already exists") \ > + /*171 */_(ER_ROWID_OVERFLOW, "Rowid is overflowed: too many entries in ephemeral space") \ > + This error message as well as check on uint64_max are not necessary, IMHO. I can not imagine how many hundreds of years a one should insert into one ephemeral table to reach this limit. Also, one extra empty line. From this patch I removed errinj and moved rowid_overflow to the next patch since it stores ids in 32 bits and here the overflow is much more feasible. Review fixes here and on the branch: ============================================================= diff --git a/src/box/errcode.h b/src/box/errcode.h index fab8b6617..04f4f34ee 100644 --- a/src/box/errcode.h +++ b/src/box/errcode.h @@ -223,8 +223,6 @@ struct errcode_record { /*168 */_(ER_DROP_FK_CONSTRAINT, "Failed to drop foreign key constraint '%s': %s") \ /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint %s does not exist") \ /*170 */_(ER_CONSTRAINT_EXISTS, "Constraint %s already exists") \ - /*171 */_(ER_ROWID_OVERFLOW, "Rowid is overflowed: too many entries in ephemeral space") \ - /* * !IMPORTANT! Please follow instructions at start of the file diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c index 075b86f9d..e810011c8 100644 --- a/src/box/memtx_space.c +++ b/src/box/memtx_space.c @@ -606,12 +606,6 @@ memtx_space_ephemeral_rowid_next(struct space *space, uint64_t *rowid) { assert(rowid != NULL); struct memtx_space *memtx_space = (struct memtx_space *)space; - ERROR_INJECT(ERRINJ_ROWID_OVERFLOW, - { memtx_space->rowid = UINT64_MAX; }); - if (unlikely(memtx_space->rowid == UINT64_MAX)) { - diag_set(ClientError, ER_ROWID_OVERFLOW); - return -1; - } *rowid = memtx_space->rowid++; return 0; } diff --git a/src/errinj.h b/src/errinj.h index 58aa5864a..84a1fbb5e 100644 --- a/src/errinj.h +++ b/src/errinj.h @@ -121,7 +121,6 @@ struct errinj { _(ERRINJ_RELAY_BREAK_LSN, ERRINJ_INT, {.iparam = -1}) \ _(ERRINJ_WAL_BREAK_LSN, ERRINJ_INT, {.iparam = -1}) \ _(ERRINJ_VY_COMPACTION_DELAY, ERRINJ_BOOL, {.bparam = false}) \ - _(ERRINJ_ROWID_OVERFLOW, ERRINJ_BOOL, {.bparam = false}) \ ENUM0(errinj_id, ERRINJ_LIST); extern struct errinj errinjs[]; diff --git a/test/box/errinj.result b/test/box/errinj.result index 191e16911..c4a1326cd 100644 --- a/test/box/errinj.result +++ b/test/box/errinj.result @@ -54,8 +54,6 @@ errinj.info() state: false ERRINJ_RELAY_REPORT_INTERVAL: state: 0 - ERRINJ_ROWID_OVERFLOW: - state: false ERRINJ_VY_READ_PAGE_TIMEOUT: state: 0 ERRINJ_XLOG_META: