From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id A2B5246970E for ; Wed, 15 Jan 2020 00:00:24 +0300 (MSK) References: <60fbcb8d59f04632ca31976bdf537afffe8e8e5f.1578951026.git.i.kosarev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <37bd8a76-11b5-e560-8bf8-acaa2539eaaf@tarantool.org> Date: Tue, 14 Jan 2020 22:00:22 +0100 MIME-Version: 1.0 In-Reply-To: <60fbcb8d59f04632ca31976bdf537afffe8e8e5f.1578951026.git.i.kosarev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 2/2] memtx: increase the memory quota if needed to truncate or delete List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ilya Kosarev , tarantool-patches@dev.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. > 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. > + 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. 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. 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.