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 9B0702EE87 for ; Wed, 14 Nov 2018 18:12:01 -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 ggQNwuMCRjaa for ; Wed, 14 Nov 2018 18:12:01 -0500 (EST) Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (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 ADED920D3A for ; Wed, 14 Nov 2018 18:12:00 -0500 (EST) From: "n.pettik" Message-Id: <2BDFF3F9-AC28-4E2F-9658-2537808FF401@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_E8D856E3-6FF5-4CCD-BC93-BFE1A52A9A99" Mime-Version: 1.0 (Mac OS X Mail 12.0 \(3445.100.39\)) Subject: [tarantool-patches] Re: [PATCH 1/2] space: add method to fetch next rowid Date: Thu, 15 Nov 2018 02:11:51 +0300 In-Reply-To: References: <11f65a415a9b1101fa4ba816be237df524de9b47.1540838910.git.korablev@tarantool.org> <52d0faf6-598d-c29b-9b9d-50f4826573eb@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: Vladislav Shpilevoy --Apple-Mail=_E8D856E3-6FF5-4CCD-BC93-BFE1A52A9A99 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On 12 Nov 2018, at 02:22, Vladislav Shpilevoy = wrote: > On 12/11/2018 02:16, n.pettik wrote: >>> On 9 Nov 2018, at 12:25, Vladislav Shpilevoy = > wrote: >>>=20 >>> 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=E2=80=99t 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=E2=80=99t 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") \ >>>> + >>>=20 >>> 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=E2=80=99t seem to be = giant. >> *It is only thoughts tho, I haven=E2=80=99t 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. >=20 > I do not mind, if you drop my fixes. It is just nitpicking. The > patchset is generally ok already. Actually, I don=E2=80=99t mind your fixes as well, so I am going to = apply them. Also, I=E2=80=99ve rebased patch-set on fresh 2.1. --Apple-Mail=_E8D856E3-6FF5-4CCD-BC93-BFE1A52A9A99 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On 12 Nov 2018, at 02:22, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
On 12/11/2018 = 02:16, n.pettik wrote:
On= 9 Nov 2018, at 12:25, Vladislav Shpilevoy <v.shpilevoy@tarantool.org <mailto:v.shpilevoy@tarantool.org>> 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=E2=80=99t 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=E2=80=99t 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=E2=80=99t seem to be giant.
*It is = only thoughts tho, I haven=E2=80=99t 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.

Actually, I don=E2=80=99t mind your fixes as well, = so I am going to apply them.
Also, I=E2=80=99ve rebased = patch-set on fresh 2.1.

= --Apple-Mail=_E8D856E3-6FF5-4CCD-BC93-BFE1A52A9A99--