Re: [Tarantool-patches] [PATCH] memtx: allow quota overuse for truncation

["Ilya Kosarev" <i.kosarev@tarantool.org>]

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


Hi!
 
Answers below. 
>Воскресенье, 20 декабря 2020, 19:13 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
> 
>> > diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
>> > index 73b4c450eb..cc431ea816 100644
>> > --- a/src/box/memtx_space.c
>> > +++ b/src/box/memtx_space.c
>> > @@ -327,8 +327,9 @@ memtx_space_execute_replace(struct space *space, struct txn *txn,
>> > struct memtx_space *memtx_space = (struct memtx_space *)space;
>> > struct txn_stmt *stmt = txn_current_stmt(txn);
>> > enum dup_replace_mode mode = dup_replace_mode(request->type);
>> > - stmt->new_tuple = memtx_tuple_new(space->format, request->tuple,
>> > - request->tuple_end);
>> > + stmt->new_tuple = space->format->vtab.tuple_new(space->format,
>> > + request->tuple,
>> > + request->tuple_end);
>>
>> 2. Seems like an expensive change. You added +2 pointer
>> dereferences to a hot path.
>>
>> Last time when you worked on that I proposed to make space_truncate
>> use box.begin + quota disable + box_upsert + quota enable + commit.
>> So there are no yields between quota enable and disable. And no changes
>> in the code not related to truncation. Why didn't it work?
>>
>> That does work, but then it was also discussed that we should use only
>> huge slabs on disabled quota, so that quota can shrink back when freed.
>
>Such discussion results are far from something trivial and obvious. Please,
>specify that in the commit message and in the comments if possible.
Right, i will repush with more info on this.
>
>Also I don't understand why can't you do that on the slab arena
>level. Start allocating only 'huge' slabs if the quota is full, but
>disabled.
Yes, allocation in case of out of quota can be done on more
internal level.
But the problem is exactly in freeing you mentioned further.
We need to free in the right way: If we are allocating with
mempool, we need to use mempool_find() properly, while
if we are allocating ‘huge’ slab, we need to free it accordingly.
So the information about the allocation has to be stored
somehow in case we are going to alter it depending on the
current state. I don’t see convenient place where we can store
it, looks like it will be even worse to store such information
in struct tuple itself.
This leads to the solution, where allocation and deletion can
only being altered per space, thus all the tuples in _truncate
space are being allocated through malloc() and freed accordingly.
>
>> It also requires specific freeing for those tuples, that is why both new() and
>> delete() are now used from vtab, which has specific version for truncate space.
>
>Why do you need special freeing? memtx_tuple_delete() does not allocate
>anything.
Commented above.
>
>> There is an option to use only specific delete() from vtab and for allocation
>> patch space_truncate() instead of new(), although it looks more strange.
>
>Didn't understand what you mean.
I mean that allocation and deletion is now altered for _truncate space.
Technically, we don’t to change the way we allocate on smalloc() and
tuple_new() levels. We can patch space_truncate() instead. But we will
still need to patch tuple_delete() for the correct freeing. This way doesn’t
look good.
>
>>
>>
>> > if (stmt->new_tuple == NULL)
>> > return -1;
>> > tuple_ref(stmt->new_tuple);
>>
>>  
>>  
>> --
>> Ilya Kosarev
>>   
 
 
--
Ilya Kosarev
 

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