[Tarantool-patches] [PATCH v2 2/2] memtx: increase the memory quota if needed to truncate or delete

[Ilya Kosarev]

Thanks for the review!
 
Ok, I see, this one is still too clumsy. Also i messed up user space
and service engines. There are 5 answers below.
 
Let’s try one more. Sent v3 with a new approach based on your idea.
 
>Среда, 15 января 2020, 0:00 +03:00 от Vladislav Shpilevoy < v.shpilevoy at tarantool.org >:
>  
>Thanks for the patch!
>
>JFI, I am still against this patch. It adds huge and
>unnecessary complexity to the code, which we will need
>to support forever. It is just not worth the pros the
>patch gives.
>
>On 13/01/2020 22:31, Ilya Kosarev wrote:
>> 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 increasing
>> memtx quota if needed for truncation or deletion. After performing it
>> quota is being set back to the previous value if possible, while it
>> should be so for almost any case, since we are meant to free some space
>> during deletion or truncation.
>>
>> Closes #3807
>> ---
>> src/box/blackhole.c | 1 +
>> src/box/box.cc | 36 +++++++++++++++++++++++++++++++++++-
>> src/box/engine.c | 11 +++++++++++
>> src/box/engine.h | 9 +++++++++
>> src/box/memtx_engine.c | 20 ++++++++++++++++++++
>> src/box/memtx_engine.h | 4 ++++
>> src/box/service_engine.c | 1 +
>> src/box/sysview.c | 1 +
>> src/box/vinyl.c | 1 +
>> 9 files changed, 83 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/box/blackhole.c b/src/box/blackhole.c
>> index 69f1deba1..af587f434 100644
>> --- a/src/box/blackhole.c
>> +++ b/src/box/blackhole.c
>> @@ -194,6 +194,7 @@ static const struct engine_vtab blackhole_engine_vtab = {
>> /* .commit_checkpoint = */ generic_engine_commit_checkpoint,
>> /* .abort_checkpoint = */ generic_engine_abort_checkpoint,
>> /* .collect_garbage = */ generic_engine_collect_garbage,
>> + /* .guarantee_memory = */ generic_engine_guarantee_memory,
>
>The only problem is with memtx engine, and I propose to solve it
>on memtx engine level. Vinyl will never need this method.
>
>(But even better I propose to drop the patch and close the issue as
>won't fix.)
>
>> /* .backup = */ generic_engine_backup,
>> /* .memory_stat = */ generic_engine_memory_stat,
>> /* .reset_stat = */ generic_engine_reset_stat,
>> diff --git a/src/box/box.cc b/src/box/box.cc
>> index 1b2b27d61..18c09ce1b 100644
>> --- a/src/box/box.cc
>> +++ b/src/box/box.cc
>> @@ -1321,9 +1341,23 @@ space_truncate(struct space *space)
>> ops_buf_end = mp_encode_uint(ops_buf_end, 1);
>> assert(ops_buf_end < buf + buf_size);
>>
>> + size_t total;
>> + bool extended = false;
>> + space->engine->vtab->guarantee_memory(space->engine,
>> + MEMTX_SLAB_SIZE,
>> + &total, &extended);
>> +
>
>Truncate is always about insertion into the memtx space _truncate.
>Here you are calling 'guarantee_memory' for the user space's engine.
>And it just won't work in case I try to truncate a vinyl space.
>
>Moreover, the encapsulation of 'memory guarantee' is broken anyway,
>because 1) you pass 'MEMTX_SLAB_SIZE' parameter to the engine's
>virtual method, 2) below you touch memtx engine explicitly.
*  
Right, this won't work in case user engine is vinyl.
>
>> if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end,
>> ops_buf, ops_buf_end, 0, NULL) != 0)
>> diag_raise();
>> +
>> + if (extended) {
>> + struct memtx_engine *memtx =
>> + (struct memtx_engine *)space->engine;
>
>Why is space->engine assumed to be memtx? This is a user's space.
>It can be vinyl.
*  
Well, extended may only be true if the engine is memtx.
>
>> + size_t new_total = quota_set(&memtx->quota, total);
>> + if (new_total > total)
>> + quota_set(&memtx->quota, quota_used(&memtx->quota));
>> + }
>> }
>
>Since this is a bug fix, there should be a regression test.
*  
I can’t really see suitable regression test.
>
>But I once again say, that it is even better to drop the patch and
>close the issue. This is just sugar, which besides may lead to a side
>effect when quota is increased, but can't be decreased back.
>
>I can't find a way how to fix it gracefully and simple.
>
>Especially for truncate which is a total disaster. The space, you have
>truncated, could be empty, but you will insert a new tuple into _truncate,
>and it will be kept, and will occupy memory. No memory is freed. And you
>won't be able to decrease the quota.
>
>AFAIR, at this moment there is no necessity in having _truncate space.
>There was something about vylog, why we added that space, and from what
>I remember, that problem has already gone. We could just drop _truncate.
>Although its existence also may be related to replication. It should be
>checked.
*  
As far as i see, _truncate space was introduced in
https://github.com/tarantool/tarantool/commit/353bcdc5d0102e20c88ad910f106156d3dd2d9da
and is needed not only for vylog handling, but also for atomic
internal truncation using trigger.
>
>Below is an idea I had recently when was thinking about the issue. I don't
>think it is good and finished, but maybe it will help you to evolve it to
>something better. We could allow to overuse quota. For example, add a flag
>to struct quota like 'is_soft'. It is false by default. When quota is not
>soft, it works just like now. When we want to allocate something above the
>quota, we set the flag to true. Now the quota is soft, and any alloc
>succeeds (unless there is no memory in the system, of course).
>
>We set the flag in memtx_space_execute_delete() before
>memtx_space->replace(space, old_tuple, NULL), and unset right after it.
>
>For truncate we set this flag in space_truncate() before box_upsert(), and
>unset right afterwards (just like you did with guarantee_memory()).
>
>That allows us to overuse quota, but don't touch quota limit. In case the
>quota is overused, and is not soft, it behaves just like when it has
>reached the limit - does not allow new allocations.
*  
Thanks for the idea! I tried to implement it in v3.
 
 
--
Ilya Kosarev
 
 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20200120/3f6a4d25/attachment.html>