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 D50CB2D9E6 for ; Sun, 11 Nov 2018 18:22:57 -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 2vtpgHbzm8sD for ; Sun, 11 Nov 2018 18:22:57 -0500 (EST) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 950E92D9E1 for ; Sun, 11 Nov 2018 18:22:57 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 1/2] space: add method to fetch next rowid References: <11f65a415a9b1101fa4ba816be237df524de9b47.1540838910.git.korablev@tarantool.org> <52d0faf6-598d-c29b-9b9d-50f4826573eb@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Mon, 12 Nov 2018 02:22:54 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: "n.pettik" , tarantool-patches@freelists.org On 12/11/2018 02:16, n.pettik wrote: > > >> On 9 Nov 2018, at 12:25, Vladislav Shpilevoy > wrote: >> >> 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. > > Vladimir didn’t suggest better solutions except for complete reworking > them. Now it is definitely bug which leads to wrong results of > SELECT queries (which is a terrible thing taking into account the fact that > SQL is supposed to be used mostly for DQL). So lets take this patch as > a workaround and rework ephemeral tables when we will have enough time > and resources (surely if Kirill and Vladimir don’t mind). > > With this bug it seems to be unacceptable to release beta version. > >> 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. > > It is true that 2^64 is likely to be quite huge number of tuples, > but for instance JOIN uses nested-loop algorithm, so it requires > n^2 memory for ephemeral table to comprise results. > In this regard, to reach the limit we need 4-way join where each > table contains 2^16 entries, which in turn doesn’t seem to be giant. > > *It is only thoughts tho, I haven’t tested it since I suppose very likely >  my pc would simply get stuck.* > > I wanted to create long test as the easiest solution, but Alexander warned > me that Travis may not survive such test due to lack of memory. > > I do not mind, if you drop my fixes. It is just nitpicking. The patchset is generally ok already.