<HTML><BODY><div>
<div class="js-helper js-readmsg-msg">
<div>
<div id="style_15786166340918237740_BODY">
<div class="class_1578688682">
<div>Thanks for the review!</div>
<div> </div>
<div>
<div>Well, yes, i first thought that it is something we don’t want to fix.<br>
However, then i decided to try it in the present way, as far as i<br>
didn't see all the trouble coming. Now i see it won't work this way.<br>
Nevertheless i think this issue is quite uncomfortable for user. Sent<br>
v2 of the patchset with approach inspired by your idea. It looks much<br>
better, however, still not sure if it is worth implementing.</div>
<div> </div>
<div> </div>
</div>
<div class="mail-quote-collapse">
<blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;"><span data-email="v.shpilevoy@tarantool.org" data-name="Vladislav Shpilevoy" data-quote-id="1695069709606419368" data-timestamp="1576715460" data-type="sender">Четверг, 19 декабря 2019, 3:31 +03:00 от Vladislav Shpilevoy <<a href="/compose?To=v.shpilevoy@tarantool.org">v.shpilevoy@tarantool.org</a>>:<br>
</span>
<div data-quote-id="1695069709606419368" data-type="body">
<div id="">
<div class="js-helper_mailru_css_attribute_postfix js-readmsg-msg_mailru_css_attribute_postfix">
<style type="text/css">
</style>
<div>
<div id="style_15767155190782026646_BODY_mailru_css_attribute_postfix">Thanks for the patch!<br>
<br>
See 3 comments below.<br>
<br>
On 13/12/2019 08:05, Ilya Kosarev wrote:<br>
> Trying to perform space:truncate() while reaching memtx_memory limit we<br>
> could experience slab allocator failure on allocation for service tuple<br>
> being upserted into service space to keep record of truncations. This<br>
> behavior seems to be quite surprising for users. Now we are using<br>
> calloc() to allocate tuples in BOX_TRUNCATE_ID space.<br>
><br>
> Closes: #3807<br>
> ---<br>
> src/box/memtx_engine.c | 50 ++++++++++++++++++++++++++++--------------<br>
> src/box/memtx_engine.h | 2 +-<br>
> src/box/memtx_space.c | 20 ++++++++++++-----<br>
> src/box/tuple.c | 16 ++++++++++----<br>
> src/box/tuple.h | 2 +-<br>
> src/box/tuple_format.c | 1 +<br>
> src/box/tuple_format.h | 10 ++++++++-<br>
> src/box/vy_stmt.c | 3 ++-<br>
> 8 files changed, 74 insertions(+), 30 deletions(-)<br>
><br>
> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c<br>
> index 23ccc4703fe..59148059f2b 100644<br>
> --- a/src/box/memtx_engine.c<br>
> +++ b/src/box/memtx_engine.c<br>
> @@ -1113,7 +1113,7 @@ memtx_leave_delayed_free_mode(struct memtx_engine *memtx)<br>
> }<br>
><br>
> struct tuple *<br>
> -memtx_tuple_new(struct tuple_format *format, const char *data, const char *end)<br>
> +memtx_tuple_new(struct tuple_format *format, const char *data, const char *end, bool use_calloc)<br>
<br>
1. Oh. No. Please. The whole point of format vtable is to avoid that.<br>
<br>
> {<br>
> struct memtx_engine *memtx = (struct memtx_engine *)format->engine;<br>
> assert(mp_typeof(*data) == MP_ARRAY);<br>
> @@ -1139,15 +1139,27 @@ memtx_tuple_new(struct tuple_format *format, const char *data, const char *end)<br>
> }<br>
><br>
> struct memtx_tuple *memtx_tuple;<br>
> - while ((memtx_tuple = smalloc(&memtx->alloc, total)) == NULL) {<br>
> - bool stop;<br>
> - memtx_engine_run_gc(memtx, &stop);<br>
> - if (stop)<br>
> - break;<br>
> - }<br>
> - if (memtx_tuple == NULL) {<br>
> - diag_set(OutOfMemory, total, "slab allocator", "memtx_tuple");<br>
> - goto end;<br>
> + if (use_calloc) {<br>
> + memtx_tuple = calloc(1, total);<br>
> + if (memtx_tuple == NULL) {<br>
> + diag_set(OutOfMemory, total, "malloc",<br>
> + "memtx_tuple");<br>
> + goto end;<br>
> + }<br>
> + format->is_malloced = true;<br>
<br>
2. Sorry, we can't change format here. The only reason why format<br>
is not const in that function is because we need increment its<br>
reference counter. A tuple can't change its own format. Especially<br>
in such a hot path.<br>
<br>
> + } else {<br>
> + while ((memtx_tuple = smalloc(&memtx->alloc,<br>
> + total)) == NULL) {<br>
> + bool stop;<br>
> + memtx_engine_run_gc(memtx, &stop);<br>
> + if (stop)<br>
> + break;<br>
> + }<br>
> + if (memtx_tuple == NULL) {<br>
> + diag_set(OutOfMemory, total, "slab allocator",<br>
> + "memtx_tuple");<br>
> + goto end;<br>
> + }<br>
> }<br>
> tuple = &memtx_tuple->base;<br>
> tuple->refs = 0;<br>
> @@ -1177,16 +1189,20 @@ memtx_tuple_delete(struct tuple_format *format, struct tuple *tuple)<br>
> struct memtx_engine *memtx = (struct memtx_engine *)format->engine;<br>
> say_debug("%s(%p)", __func__, tuple);<br>
> assert(tuple->refs == 0);<br>
> - tuple_format_unref(format);<br>
> struct memtx_tuple *memtx_tuple =<br>
> container_of(tuple, struct memtx_tuple, base);<br>
> size_t total = tuple_size(tuple) + offsetof(struct memtx_tuple, base);<br>
> - if (memtx->alloc.free_mode != SMALL_DELAYED_FREE ||<br>
> - memtx_tuple->version == memtx->snapshot_version ||<br>
> - format->is_temporary)<br>
> - smfree(&memtx->alloc, memtx_tuple, total);<br>
> - else<br>
> - smfree_delayed(&memtx->alloc, memtx_tuple, total);<br>
> + if (format->is_malloced) {<br>
> + free(memtx_tuple);<br>
<br>
3. There is a certain purpose for these free_mode, version, blah<br>
blah. These attributes create a read view of the whole memtx engine<br>
to make a snapshot, but do not stop new requests. Tuples are not<br>
deleted during snapshot.<br>
<br>
Having ignored the free mode you get this:<br>
<br>
====================================================================<br>
<br>
box.cfg{}<br>
errinj = box.error.injection<br>
fiber = require('fiber')<br>
<br>
s = box.schema.create_space('test')<br>
pk = s:create_index('pk')<br>
s:truncate()<br>
s:replace{1}<br>
box.error.injection.set('ERRINJ_SNAP_WRITE_DELAY', true)<br>
f = fiber.create(box.snapshot)<br>
box.space._truncate:delete{s.id}<br>
<br>
-- The code below improves reproducibility. It creates and<br>
-- deletes several tuples to unref the blessed tuple<br>
-- (tuple_bless), rewrite some recently freed heap chunks.<br>
<br>
s:get{1}<br>
collectgarbage('collect')<br>
s:replace{1}<br>
s:get{1}<br>
collectgarbage('collect')<br>
s:replace{1}<br>
s:get{1}<br>
collectgarbage('collect')<br>
s:replace{1}<br>
s:get{1}<br>
collectgarbage('collect')<br>
<br>
--<br>
<br>
box.error.injection.set('ERRINJ_SNAP_WRITE_DELAY', false)<br>
<br>
tarantool> Process 94390 stopped<br>
* thread #2, name = 'snapshot', stop reason = EXC_BAD_ACCESS (code=2, address=0x105481000)<br>
frame #0: 0x00007fff5a964ce9 libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell + 41<br>
libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell:<br>
-> 0x7fff5a964ce9 <+41>: rep movsb (%rsi), %es:(%rdi)<br>
0x7fff5a964ceb <+43>: popq %rbp<br>
0x7fff5a964cec <+44>: retq<br>
0x7fff5a964ced <+45>: cmpq %rdi, %rsi<br>
Target 0: (tarantool) stopped.<br>
(lldb) bt<br>
* thread #2, name = 'snapshot', stop reason = EXC_BAD_ACCESS (code=2, address=0x105481000)<br>
* frame #0: 0x00007fff5a964ce9 libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell + 41<br>
frame #1: 0x00000001003f56ba tarantool`obuf_dup(buf=0x000000010f0ff8b0, data=0x0000000105157aa6, size=85071264) at obuf.c:161:2<br>
frame #2: 0x00000001003634e8 tarantool`xlog_write_row(log=0x000000010f0ff208, packet=0x000000010f0ff0d8) at xlog.c:1320:7<br>
frame #3: 0x0000000100038aef tarantool`checkpoint_write_row(l=0x000000010f0ff208, row=0x000000010f0ff0d8) at memtx_engine.c:431:20<br>
frame #4: 0x0000000100038a2e tarantool`checkpoint_write_tuple(l=0x000000010f0ff208, space_id=330, group_id=0, data="", size=85071264) at memtx_engine.c:459:9<br>
frame #5: 0x0000000100038811 tarantool`checkpoint_f(ap=0x000000010f000278) at memtx_engine.c:603:8<br>
frame #6: 0x0000000100005e5a tarantool`fiber_cxx_invoke(f=(tarantool`checkpoint_f at memtx_engine.c:577), ap=0x000000010f000278)(__va_list_tag*), __va_list_tag*) at fiber.h:762:10<br>
frame #7: 0x000000010018168b tarantool`fiber_loop(data=0x0000000000000000) at fiber.c:830:18<br>
frame #8: 0x00000001004165e7 tarantool`coro_init at coro.c:110:3<br>
<br>
====================================================================<br>
<br>
Snapshoting means that you can't solve the issue on the format<br>
level, or tuple level. You need to patch the engine level (IMO).<br>
<br>
Honestly, I thought about this issue for quite a long time already,<br>
and I don't have a plan how to fix it. All ways look bad. All of them<br>
assume special patching of _truncate space insertion and crutches for<br>
deletion. For me the most promising way was to introduce an analogue<br>
of vy_quota_force_use() somehow, for _truncate, and use it in a<br>
before_replace trigger of _truncate. Forced use of quota would allow<br>
to temporary overflow the memory limit. But it won't fix deletion.<br>
Maybe the key is in introduction of quota overuse for deletions. I<br>
don't know.<br>
<br>
Personally, I don't think the issue is worth fixing at all. If a<br>
user wants to delete/truncate, he can increase memtx memory, do<br>
cleanup, and decrease it back.</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
<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>
<hr>
<div> </div>
<div data-signature-widget="container">
<div data-signature-widget="content">
<div>--<br>
Ilya Kosarev</div>
</div>
</div>
<div> </div>
</div>
<style type="text/css">
</style>
</BODY></HTML>