From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: Ilya Kosarev <i.kosarev@tarantool.org>, tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v2 2/2] memtx: increase the memory quota if needed to truncate or delete Date: Tue, 14 Jan 2020 22:00:22 +0100 [thread overview] Message-ID: <37bd8a76-11b5-e560-8bf8-acaa2539eaaf@tarantool.org> (raw) In-Reply-To: <60fbcb8d59f04632ca31976bdf537afffe8e8e5f.1578951026.git.i.kosarev@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.
next prev parent reply other threads:[~2020-01-14 21:00 UTC|newest] Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-01-13 21:31 [Tarantool-patches] [PATCH v2 0/2] Safe truncation and deletion Ilya Kosarev 2020-01-13 21:31 ` [Tarantool-patches] [PATCH v2 1/2] b-tree: return NULL on matras_alloc fail Ilya Kosarev 2020-01-14 21:00 ` Vladislav Shpilevoy 2020-01-20 18:13 ` Ilya Kosarev 2020-01-13 21:31 ` [Tarantool-patches] [PATCH v2 2/2] memtx: increase the memory quota if needed to truncate or delete Ilya Kosarev 2020-01-14 21:00 ` Vladislav Shpilevoy [this message] 2020-01-20 18:13 ` Ilya Kosarev 2020-01-24 11:21 ` Konstantin Osipov -- strict thread matches above, loose matches on Subject: below -- 2020-01-10 0:36 [Tarantool-patches] [PATCH v2 0/2] Safe truncation and deletion Ilya Kosarev 2020-01-10 0:36 ` [Tarantool-patches] [PATCH v2 2/2] memtx: increase the memory quota if needed to truncate or delete Ilya Kosarev
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=37bd8a76-11b5-e560-8bf8-acaa2539eaaf@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=i.kosarev@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 2/2] memtx: increase the memory quota if needed to truncate or delete' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox