[tarantool-patches] Re: [PATCH 1/2] space: add method to fetch next rowid

["n.pettik" <korablev@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 3816 bytes --]



> 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’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.

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


[-- Attachment #2: Type: text/html, Size: 7814 bytes --]