

<HTML><BODY><div><div>Hi!<br> </div><div>Thanks for your review.</div><div> </div><div>2 answers below.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Вторник, 15 декабря 2020, 2:41 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16079893151349558396_BODY">Thanks for the patch!<br><br>See 2 comments below.<br><br>On 11.12.2020 16:37, Ilya Kosarev wrote:<br>> Trying to perform space:truncate() while reaching memtx_memory limit<br>> we could experience slab allocator failure. This behavior seems to be<br>> quite surprising for users. Now we are allowing to overuse memtx quota<br>> for tuples in space _truncate using flag in struct quota.<br>> Truncate tuples are only being allocated with large slabs using malloc<br>> so that the quota can shrink back when they are freed.<br><br>1. 3807 is also about delete. Why didn't you patch it too? AFAIR, the<br>fix I proposed was easy - in case something inside memtx_space_execute_delete()<br>fails due to OOM, we disable quota, try again, and enable quota. In fact,<br>it would be even simpler than the truncation fix, I suppose.</div></div></div></div></blockquote></div><div>There is no reproduce for delete() failure. And we discussed back in February</div><div>that delete() has to work through reserves. I see that there is theoretical possibility</div><div>for reserves to be exhausted, but i can’t see the exact sequence of actions to reach it.</div><div>Thus i think it is not ok to use quota disabling for delete().</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> Closes #3807<br>> ---<br>> Branch: <a href="https://github.com/tarantool/tarantool/tree/i.kosarev/gh-3807-safe-alloc-on-truncation" target="_blank">https://github.com/tarantool/tarantool/tree/i.kosarev/gh-3807-safe-alloc-on-truncation</a><br>> Issue: <a href="https://github.com/tarantool/tarantool/issues/3807" target="_blank">https://github.com/tarantool/tarantool/issues/3807</a><br>><br>> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c<br>> index 73b4c450eb..cc431ea816 100644<br>> --- a/src/box/memtx_space.c<br>> +++ b/src/box/memtx_space.c<br>> @@ -327,8 +327,9 @@ memtx_space_execute_replace(struct space *space, struct txn *txn,<br>> struct memtx_space *memtx_space = (struct memtx_space *)space;<br>> struct txn_stmt *stmt = txn_current_stmt(txn);<br>> enum dup_replace_mode mode = dup_replace_mode(request->type);<br>> - stmt->new_tuple = memtx_tuple_new(space->format, request->tuple,<br>> - request->tuple_end);<br>> + stmt->new_tuple = space->format->vtab.tuple_new(space->format,<br>> + request->tuple,<br>> + request->tuple_end);<br><br>2. Seems like an expensive change. You added +2 pointer<br>dereferences to a hot path.<br><br>Last time when you worked on that I proposed to make space_truncate<br>use box.begin + quota disable + box_upsert + quota enable + commit.<br>So there are no yields between quota enable and disable. And no changes<br>in the code not related to truncation. Why didn't it work?</div></div></div></div></blockquote></div><div>That does work, but then it was also discussed that we should use only</div><div>huge slabs on disabled quota, so that quota can shrink back when freed.</div><div>It also requires specific freeing for those tuples, that is why both new() and</div><div>delete() are now used from vtab, which has specific version for truncate space.</div><div>There is an option to use only specific delete() from vtab and for allocation</div><div>patch space_truncate() instead of new(), although it looks more strange.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> if (stmt->new_tuple == NULL)<br>> return -1;<br>> tuple_ref(stmt->new_tuple);</div></div></div></div></blockquote> <div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Ilya Kosarev</div></div></div><div> </div></div></BODY></HTML>