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