

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