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

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

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


Hi!
 
Thanks for your review.
 
2 answers below. 
>Вторник, 15 декабря 2020, 2:41 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
> 
>Thanks for the patch!
>
>See 2 comments below.
>
>On 11.12.2020 16:37, Ilya Kosarev wrote:
>> Trying to perform space:truncate() while reaching memtx_memory limit
>> we could experience slab allocator failure. This behavior seems to be
>> quite surprising for users. Now we are allowing to overuse memtx quota
>> for tuples in space _truncate using flag in struct quota.
>> Truncate tuples are only being allocated with large slabs using malloc
>> so that the quota can shrink back when they are freed.
>
>1. 3807 is also about delete. Why didn't you patch it too? AFAIR, the
>fix I proposed was easy - in case something inside memtx_space_execute_delete()
>fails due to OOM, we disable quota, try again, and enable quota. In fact,
>it would be even simpler than the truncation fix, I suppose.
There is no reproduce for delete() failure. And we discussed back in February
that delete() has to work through reserves. I see that there is theoretical possibility
for reserves to be exhausted, but i can’t see the exact sequence of actions to reach it.
Thus i think it is not ok to use quota disabling for delete().
>
>> Closes #3807
>> ---
>> Branch:  https://github.com/tarantool/tarantool/tree/i.kosarev/gh-3807-safe-alloc-on-truncation
>> Issue:  https://github.com/tarantool/tarantool/issues/3807
>>
>> 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.
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.
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.
>
>> if (stmt->new_tuple == NULL)
>> return -1;
>> tuple_ref(stmt->new_tuple); 
 
 
--
Ilya Kosarev
 

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