[Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion

[Ilya Kosarev]

Thanks for your review!

Sent tarantool/small patch and v4 of this patchset considering the
drawbacks. 
>Вторник, 21 января 2020, 23:59 +03:00 от Vladislav Shpilevoy <v.shpilevoy at tarantool.org>:
> 
>Thanks for the patch!
>
>I answer some of Nikita's questions below and
>provide my own review further.
>
>See 7 comments below.
>
>On 21/01/2020 12:42, Nikita Pettik wrote:
>> On 20 Jan 21:13, Ilya Kosarev wrote:
>>> diff --git a/src/box/box.cc b/src/box/box.cc
>>> index 1b2b27d61..678c9ffe3 100644
>>> --- a/src/box/box.cc
>>> +++ b/src/box/box.cc
>>> @@ -1321,9 +1321,14 @@ space_truncate(struct space *space)
>>> ops_buf_end = mp_encode_uint(ops_buf_end, 1);
>>> assert(ops_buf_end < buf + buf_size);
>>>
>>> + struct space *truncate_space = space_cache_find_xc(BOX_TRUNCATE_ID);
>>> + memtx_engine_set_quota_strictness(truncate_space->engine, false);
>>
>> I wouldn't call it strictness. In fact you simply turn quota on/off.
>> So I'd better call it quota_enable/disable.
>>
>> Nit: please accompany your code with (at least brief) comments.
>
>1. On/off sounds good.
>
>>> +
>>> if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end,
>>> ops_buf, ops_buf_end, 0, NULL) != 0)
>>> diag_raise();
>>
>> If box_upsert() fails, quota is not re-enabled, isn't it?
>
>2. This is a bug, yes. But not the only one. The upsert will write
>to WAL and yield. During that yield the quota is turned off. And
>any other fiber can interfere and overuse the quota even more with
>something not related to delete/truncate.
>
>This can be fixed, for example, by making txn_begin() + quota_off() +
>upsert() + quota_on() + commit(). Also we could just not touch truncate
>and fix delete() only. Delete() can never leave the quota overused,
>because is not stored anywhere in memtx. While truncate is stored in
>_truncate. And delete would be sufficient, because can be used to free
>the quota enough so as to call truncate()/whatever else the user wanted.
>
>>> +
>>> + memtx_engine_set_quota_strictness(truncate_space->engine, true);
>>> }
>>>
>>> int
>>> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
>>> index 4da80824a..c19205d34 100644
>>> --- a/src/box/memtx_engine.c
>>> +++ b/src/box/memtx_engine.c
>>> @@ -1090,6 +1090,15 @@ memtx_engine_set_memory(struct memtx_engine *memtx, size_t size)
>>> return 0;
>>> }
>>>
>>> +void
>>> +memtx_engine_set_quota_strictness(struct engine *engine, bool strict)
>>> +{
>>> + if (strncmp(engine->name, "memtx", 5) != 0)
>>> + return;
>>
>> memtx_ prefix in the name of this function assumes that it is called
>> only for memtx engine. Note that there's no such checks in memtx_engine
>> interface functions.
>>
>>> + struct memtx_engine *memtx = (struct memtx_engine *)engine;
>>> + quota_set_strictness(&memtx->quota, strict);
>>
>> What happens if quota is turned on back, but it still exceeds limit?
>> I guess it would turn out to be in inconsistent state, since in
>> normal operation mode there's no chance of being beyond the limit.
>
>3. That problem is not solvable. There is always a chance, that truncate
>won't free any memory (for example, the target space could be empty
>already). If, after the quota is back on, it appears to be overused, nothing
>can be done with it. It will stay overused until more delete/truncate will be
>executed and actually delete something, or the user will increase memtx memory.
>
>> commit 45d9f491f91d93c8835ca725c234f7113c0d2aa8
>> Author: Ilya Kosarev < i.kosarev at tarantool.org >
>> Date: Mon Jan 20 20:21:37 2020 +0300
>>
>> memtx: allow quota overuse for truncation and deletion
>>
>>
>> Trying to perform space:truncate() and space:delete() 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 if needed for truncation or deletion, using flag
>> in struct quota. After performing truncation or deletion we reset the
>> flag so that quota can be overused any more.
>>
>> Closes #3807
>>
>> diff --git a/src/box/box.cc b/src/box/box.cc
>> index 1b2b27d61..678c9ffe3 100644
>> --- a/src/box/box.cc
>> +++ b/src/box/box.cc
>> @@ -1321,9 +1321,14 @@ space_truncate(struct space *space)
>> ops_buf_end = mp_encode_uint(ops_buf_end, 1);
>> assert(ops_buf_end < buf + buf_size);
>>
>> + struct space *truncate_space = space_cache_find_xc(BOX_TRUNCATE_ID);
>> + memtx_engine_set_quota_strictness(truncate_space->engine, false);
>> +
>> if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end,
>> ops_buf, ops_buf_end, 0, NULL) != 0)
>> diag_raise();
>> +
>> + memtx_engine_set_quota_strictness(truncate_space->engine, true);
>> }
>
>4. IMO, we should not touch quota at all until we see a not empty diag
>with OOM error type in it. Both here and in delete. Truncate is not
>a frequent operation (although it may be in some rare installations),
>but delete surely should not touch quota each time.
>
>> int
>> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
>> index 4da80824a..c19205d34 100644
>> --- a/src/box/memtx_engine.c
>> +++ b/src/box/memtx_engine.c
>> @@ -1090,6 +1090,15 @@ memtx_engine_set_memory(struct memtx_engine *memtx, size_t size)
>> return 0;
>> }
>>
>> +void
>> +memtx_engine_set_quota_strictness(struct engine *engine, bool strict)
>> +{
>> + if (strncmp(engine->name, "memtx", 5) != 0)
>> + return;
>
>5. That check should not exist. It slows down the caller, and
>in upper code you always know whether you work with memtx or
>not. So it makes no sense to check it here. Truncate is
>always about insertion into a memtx space. Delete touches qouta
>inside memtx_space implementation. No chances that a vinyl space
>would call this function.
>
>> + struct memtx_engine *memtx = (struct memtx_engine *)engine;
>> + quota_set_strictness(&memtx->quota, strict);
>> +}
>> +
>> void
>> memtx_engine_set_max_tuple_size(struct memtx_engine *memtx, size_t max_size)
>> {
>> diff --git a/src/lib/small b/src/lib/small
>> index 50cb78743..bdcd569f9 160000
>> --- a/src/lib/small
>> +++ b/src/lib/small
>> @@ -1 +1 @@
>> -Subproject commit 50cb7874380b286f3b34c82299cf5eb88b0d0059
>> +Subproject commit bdcd569f9ed753efb805f708089ca86e2520a44f
>
>6. Please, write tests. They should include the bug test itself, and
>complex things such as quota overuse kept in case truncate() didn't
>free anything. You can use memtx_memory parameter to make it small
>enough so as the test would not take much time.
>
>7. Seems like you changed something in small/. You need to send it as
>a separate patch with its own tests, and all. 
 
 
--
Ilya Kosarev
 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20200214/a7f4ffb4/attachment.html>