* [Tarantool-patches] [PATCH v3 0/2] Safe truncation and deletion @ 2020-01-20 18:13 Ilya Kosarev 2020-01-20 18:13 ` [Tarantool-patches] [PATCH v3 1/2] b-tree: return NULL on matras_alloc fail Ilya Kosarev 2020-01-20 18:13 ` [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion Ilya Kosarev 0 siblings, 2 replies; 27+ messages in thread From: Ilya Kosarev @ 2020-01-20 18:13 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy space:truncate() and space:delete() could fail on memory allocations when reaching memtx_memory limit. As far as it is quite an ill behaviour, it is fixed in this patchset through memtx quota strictness adjustment. Now it can be overused if needed. Also possible bps_tree_create_leaf NULL dereference issue is fixed. Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-3807-safe-alloc-on-truncation Issue: https://github.com/tarantool/tarantool/issues/3807 Changes in v2: Approach changed completely: now we are not trying to allocate service tuples in some safe way, but increasing memtx quota so that space:truncate() and space:delete() won't fail on allocation. Changes in v3: Now we are not increasing memtx quota. Instead we just set a flag to allow quota overuse for space:truncate() and space:delete(). Ilya Kosarev (2): b-tree: return NULL on matras_alloc fail memtx: allow quota overuse for truncation and deletion src/box/box.cc | 5 +++++ src/box/memtx_engine.c | 9 +++++++++ src/box/memtx_engine.h | 3 +++ src/box/memtx_space.c | 13 +++++++++---- src/lib/salad/bps_tree.h | 7 +++++-- src/lib/small | 2 +- 6 files changed, 32 insertions(+), 7 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [Tarantool-patches] [PATCH v3 1/2] b-tree: return NULL on matras_alloc fail 2020-01-20 18:13 [Tarantool-patches] [PATCH v3 0/2] Safe truncation and deletion Ilya Kosarev @ 2020-01-20 18:13 ` Ilya Kosarev 2020-01-21 10:32 ` Nikita Pettik 2020-01-21 20:55 ` Vladislav Shpilevoy 2020-01-20 18:13 ` [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion Ilya Kosarev 1 sibling, 2 replies; 27+ messages in thread From: Ilya Kosarev @ 2020-01-20 18:13 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy In bps_tree_create_leaf we use matras_alloc in case bps_tree_garbage_pop didn't work out. However it also might not succeed. Then we need to return NULL instead of dereferencing NULL pointer. Part of #3807 --- src/lib/salad/bps_tree.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/lib/salad/bps_tree.h b/src/lib/salad/bps_tree.h index d28b53f53..a10f0a32c 100644 --- a/src/lib/salad/bps_tree.h +++ b/src/lib/salad/bps_tree.h @@ -2147,8 +2147,11 @@ bps_tree_create_leaf(struct bps_tree *tree, bps_tree_block_id_t *id) { struct bps_leaf *res = (struct bps_leaf *) bps_tree_garbage_pop(tree, id); - if (!res) - res = (struct bps_leaf *)matras_alloc(&tree->matras, id); + if (res == NULL) { + res = (struct bps_leaf *) matras_alloc(&tree->matras, id); + if (res == NULL) + return NULL; + } res->header.type = BPS_TREE_BT_LEAF; tree->leaf_count++; return res; -- 2.17.1 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 1/2] b-tree: return NULL on matras_alloc fail 2020-01-20 18:13 ` [Tarantool-patches] [PATCH v3 1/2] b-tree: return NULL on matras_alloc fail Ilya Kosarev @ 2020-01-21 10:32 ` Nikita Pettik 2020-01-31 8:18 ` Konstantin Osipov 2020-01-21 20:55 ` Vladislav Shpilevoy 1 sibling, 1 reply; 27+ messages in thread From: Nikita Pettik @ 2020-01-21 10:32 UTC (permalink / raw) To: Ilya Kosarev; +Cc: tarantool-patches, v.shpilevoy On 20 Jan 21:13, Ilya Kosarev wrote: > In bps_tree_create_leaf we use matras_alloc in case > bps_tree_garbage_pop didn't work out. However it also might not > succeed. Then we need to return NULL instead of dereferencing NULL > pointer. > > Part of #3807 > --- > src/lib/salad/bps_tree.h | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/lib/salad/bps_tree.h b/src/lib/salad/bps_tree.h > index d28b53f53..a10f0a32c 100644 > --- a/src/lib/salad/bps_tree.h > +++ b/src/lib/salad/bps_tree.h > @@ -2147,8 +2147,11 @@ bps_tree_create_leaf(struct bps_tree *tree, bps_tree_block_id_t *id) > { What about bps_tree_create_inner()? It follows the same allocation pattern. Doesn't it require same NULL check? What is more, result of bps_tree_create_leaf() invokation is not checked in bps_tree_process_insert_leaf(). > struct bps_leaf *res = (struct bps_leaf *) > bps_tree_garbage_pop(tree, id); > - if (!res) > - res = (struct bps_leaf *)matras_alloc(&tree->matras, id); > + if (res == NULL) { Nit: personally I wouldn't strive to fix codestyle in this place since all bps_tree.h code follows the same (even if a bit different) codestyle. So now code being fixed looks a bit strange (IMHO). > + res = (struct bps_leaf *) matras_alloc(&tree->matras, id); > + if (res == NULL) > + return NULL; > + } > res->header.type = BPS_TREE_BT_LEAF; > tree->leaf_count++; > return res; > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 1/2] b-tree: return NULL on matras_alloc fail 2020-01-21 10:32 ` Nikita Pettik @ 2020-01-31 8:18 ` Konstantin Osipov 2020-02-04 17:13 ` Nikita Pettik 0 siblings, 1 reply; 27+ messages in thread From: Konstantin Osipov @ 2020-01-31 8:18 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches, v.shpilevoy * Nikita Pettik <korablev@tarantool.org> [20/01/21 13:37]: > On 20 Jan 21:13, Ilya Kosarev wrote: > > In bps_tree_create_leaf we use matras_alloc in case > > bps_tree_garbage_pop didn't work out. However it also might not > > succeed. Then we need to return NULL instead of dereferencing NULL > > pointer. I don't understand the attempt to fix it. The reason the allocations are not checked - most likely -because BPS should refuse to even begin an operation if there is not enough memory in matras. Most likely Alexander Lyapunov was relying on that, and this is why you don't have these checks all over bps code. -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 1/2] b-tree: return NULL on matras_alloc fail 2020-01-31 8:18 ` Konstantin Osipov @ 2020-02-04 17:13 ` Nikita Pettik 2020-02-04 17:25 ` Konstantin Osipov 0 siblings, 1 reply; 27+ messages in thread From: Nikita Pettik @ 2020-02-04 17:13 UTC (permalink / raw) To: Konstantin Osipov, Ilya Kosarev, tarantool-patches, v.shpilevoy On 31 Jan 11:18, Konstantin Osipov wrote: > * Nikita Pettik <korablev@tarantool.org> [20/01/21 13:37]: > > On 20 Jan 21:13, Ilya Kosarev wrote: > > > In bps_tree_create_leaf we use matras_alloc in case > > > bps_tree_garbage_pop didn't work out. However it also might not > > > succeed. Then we need to return NULL instead of dereferencing NULL > > > pointer. > > I don't understand the attempt to fix it. > > The reason the allocations are not checked - most likely -because > BPS should refuse to even begin an operation if there is not > enough memory in matras. According to the code it doesn't look so. Matras allocation checked on fails, except two ones in bps_tree_create_inner() and bps_tree_create_leaf(). > Most likely Alexander Lyapunov was relying on that, and this is why you > don't have these checks all over bps code.> > > -- > Konstantin Osipov, Moscow, Russia ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 1/2] b-tree: return NULL on matras_alloc fail 2020-02-04 17:13 ` Nikita Pettik @ 2020-02-04 17:25 ` Konstantin Osipov 2020-02-04 18:08 ` Nikita Pettik 0 siblings, 1 reply; 27+ messages in thread From: Konstantin Osipov @ 2020-02-04 17:25 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches, v.shpilevoy * Nikita Pettik <korablev@tarantool.org> [20/02/04 20:21]: > > > On 20 Jan 21:13, Ilya Kosarev wrote: > > > > In bps_tree_create_leaf we use matras_alloc in case > > > > bps_tree_garbage_pop didn't work out. However it also might not > > > > succeed. Then we need to return NULL instead of dereferencing NULL > > > > pointer. > > > > I don't understand the attempt to fix it. > > > > The reason the allocations are not checked - most likely -because > > BPS should refuse to even begin an operation if there is not > > enough memory in matras. > > According to the code it doesn't look so. Matras allocation checked > on fails, except two ones in bps_tree_create_inner() and > bps_tree_create_leaf(). Because reserve_blocks() should reserve enough blocks or fail, and it is called before create_inner/create_leaf in all execution paths? -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 1/2] b-tree: return NULL on matras_alloc fail 2020-02-04 17:25 ` Konstantin Osipov @ 2020-02-04 18:08 ` Nikita Pettik 2020-02-04 18:25 ` Konstantin Osipov 0 siblings, 1 reply; 27+ messages in thread From: Nikita Pettik @ 2020-02-04 18:08 UTC (permalink / raw) To: Konstantin Osipov, Ilya Kosarev, tarantool-patches, v.shpilevoy On 04 Feb 20:25, Konstantin Osipov wrote: > * Nikita Pettik <korablev@tarantool.org> [20/02/04 20:21]: > > > > On 20 Jan 21:13, Ilya Kosarev wrote: > > > > > In bps_tree_create_leaf we use matras_alloc in case > > > > > bps_tree_garbage_pop didn't work out. However it also might not > > > > > succeed. Then we need to return NULL instead of dereferencing NULL > > > > > pointer. > > > > > > I don't understand the attempt to fix it. > > > > > > The reason the allocations are not checked - most likely -because > > > BPS should refuse to even begin an operation if there is not > > > enough memory in matras. > > > > According to the code it doesn't look so. Matras allocation checked > > on fails, except two ones in bps_tree_create_inner() and > > bps_tree_create_leaf(). > > Because reserve_blocks() should reserve enough blocks or fail, and > it is called before create_inner/create_leaf in all execution paths? Consider following path: memtx_tree_index_replace | ->memtx_tree_insert (bps_tree_insert) | ->bps_tree_insert_first_elem | ->bps_tree_create_leaf | -> matras_alloc In this case reserve_blocks() is not called. AFAIU path is likely to be reachable. Am I missing smth? > -- > Konstantin Osipov, Moscow, Russia ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 1/2] b-tree: return NULL on matras_alloc fail 2020-02-04 18:08 ` Nikita Pettik @ 2020-02-04 18:25 ` Konstantin Osipov 0 siblings, 0 replies; 27+ messages in thread From: Konstantin Osipov @ 2020-02-04 18:25 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches, v.shpilevoy * Nikita Pettik <korablev@tarantool.org> [20/02/04 21:10]: > On 04 Feb 20:25, Konstantin Osipov wrote: > > * Nikita Pettik <korablev@tarantool.org> [20/02/04 20:21]: > > > > > On 20 Jan 21:13, Ilya Kosarev wrote: > > > > > > In bps_tree_create_leaf we use matras_alloc in case > > > > > > bps_tree_garbage_pop didn't work out. However it also might not > > > > > > succeed. Then we need to return NULL instead of dereferencing NULL > > > > > > pointer. > > > > > > > > I don't understand the attempt to fix it. > > > > > > > > The reason the allocations are not checked - most likely -because > > > > BPS should refuse to even begin an operation if there is not > > > > enough memory in matras. > > > > > > According to the code it doesn't look so. Matras allocation checked > > > on fails, except two ones in bps_tree_create_inner() and > > > bps_tree_create_leaf(). > > > > Because reserve_blocks() should reserve enough blocks or fail, and > > it is called before create_inner/create_leaf in all execution paths? > > Consider following path: > > memtx_tree_index_replace > | > ->memtx_tree_insert (bps_tree_insert) > | > ->bps_tree_insert_first_elem > | > ->bps_tree_create_leaf > | > -> matras_alloc > > In this case reserve_blocks() is not called. AFAIU path is likely to > be reachable. Am I missing smth? No, in this trace I agree with you, we clearly see some sort of refactoring artefact or a coding bug. What I am observing is that bps_tree_insert_first_elem() actually checks the return value of bps_tree_create_leaf(). But bps_tree_create_leaf() never returns NULL, so this check is never false. bps was originally written in C++, so all of these checks for OOm were added when it was rewritten in C, after the fact. Looks like we're dealing with an artefact of this rewrite - the checks are not consistent. -- Konstantin Osipov, Moscow, Russia https://scylladb.com ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 1/2] b-tree: return NULL on matras_alloc fail 2020-01-20 18:13 ` [Tarantool-patches] [PATCH v3 1/2] b-tree: return NULL on matras_alloc fail Ilya Kosarev 2020-01-21 10:32 ` Nikita Pettik @ 2020-01-21 20:55 ` Vladislav Shpilevoy 1 sibling, 0 replies; 27+ messages in thread From: Vladislav Shpilevoy @ 2020-01-21 20:55 UTC (permalink / raw) To: Ilya Kosarev, tarantool-patches Hi! Thanks for the patch! I agree with Nikita in everything, especially about bps_tree_create_inner(). Sorry, when I said that code style should be fixed I didn't take into account that bps_tree is like a separate library with own code style where ! is used instead of == NULL. On 20/01/2020 19:13, Ilya Kosarev wrote: > In bps_tree_create_leaf we use matras_alloc in case > bps_tree_garbage_pop didn't work out. However it also might not > succeed. Then we need to return NULL instead of dereferencing NULL > pointer. > > Part of #3807 > --- > src/lib/salad/bps_tree.h | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/lib/salad/bps_tree.h b/src/lib/salad/bps_tree.h > index d28b53f53..a10f0a32c 100644 > --- a/src/lib/salad/bps_tree.h > +++ b/src/lib/salad/bps_tree.h > @@ -2147,8 +2147,11 @@ bps_tree_create_leaf(struct bps_tree *tree, bps_tree_block_id_t *id) > { > struct bps_leaf *res = (struct bps_leaf *) > bps_tree_garbage_pop(tree, id); > - if (!res) > - res = (struct bps_leaf *)matras_alloc(&tree->matras, id); > + if (res == NULL) { > + res = (struct bps_leaf *) matras_alloc(&tree->matras, id); > + if (res == NULL) > + return NULL; > + } > res->header.type = BPS_TREE_BT_LEAF; > tree->leaf_count++; > return res; > ^ permalink raw reply [flat|nested] 27+ messages in thread
* [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-01-20 18:13 [Tarantool-patches] [PATCH v3 0/2] Safe truncation and deletion Ilya Kosarev 2020-01-20 18:13 ` [Tarantool-patches] [PATCH v3 1/2] b-tree: return NULL on matras_alloc fail Ilya Kosarev @ 2020-01-20 18:13 ` Ilya Kosarev 2020-01-21 11:42 ` Nikita Pettik 2020-01-31 8:21 ` Konstantin Osipov 1 sibling, 2 replies; 27+ messages in thread From: Ilya Kosarev @ 2020-01-20 18:13 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy 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 allowing to overuse memtx quota if needed for truncation or deletion, using flag in struct quota. After performing truncation or deletion we reset the flag so that quota can be overused any more. Closes #3807 --- src/box/box.cc | 5 +++++ src/box/memtx_engine.c | 9 +++++++++ src/box/memtx_engine.h | 3 +++ src/box/memtx_space.c | 13 +++++++++---- src/lib/small | 2 +- 5 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/box/box.cc b/src/box/box.cc index 1b2b27d61..678c9ffe3 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -1321,9 +1321,14 @@ space_truncate(struct space *space) ops_buf_end = mp_encode_uint(ops_buf_end, 1); assert(ops_buf_end < buf + buf_size); + struct space *truncate_space = space_cache_find_xc(BOX_TRUNCATE_ID); + memtx_engine_set_quota_strictness(truncate_space->engine, false); + if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end, ops_buf, ops_buf_end, 0, NULL) != 0) diag_raise(); + + memtx_engine_set_quota_strictness(truncate_space->engine, true); } int diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c index 4da80824a..c19205d34 100644 --- a/src/box/memtx_engine.c +++ b/src/box/memtx_engine.c @@ -1090,6 +1090,15 @@ memtx_engine_set_memory(struct memtx_engine *memtx, size_t size) return 0; } +void +memtx_engine_set_quota_strictness(struct engine *engine, bool strict) +{ + if (strncmp(engine->name, "memtx", 5) != 0) + return; + struct memtx_engine *memtx = (struct memtx_engine *)engine; + quota_set_strictness(&memtx->quota, strict); +} + void memtx_engine_set_max_tuple_size(struct memtx_engine *memtx, size_t max_size) { diff --git a/src/box/memtx_engine.h b/src/box/memtx_engine.h index f562c66df..0793fc2ab 100644 --- a/src/box/memtx_engine.h +++ b/src/box/memtx_engine.h @@ -213,6 +213,9 @@ memtx_engine_set_snap_io_rate_limit(struct memtx_engine *memtx, double limit); int memtx_engine_set_memory(struct memtx_engine *memtx, size_t size); +void +memtx_engine_set_quota_strictness(struct engine *engine, bool strict); + void memtx_engine_set_max_tuple_size(struct memtx_engine *memtx, size_t max_size); diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c index 6ef84e045..20ce2f974 100644 --- a/src/box/memtx_space.c +++ b/src/box/memtx_space.c @@ -354,10 +354,15 @@ memtx_space_execute_delete(struct space *space, struct txn *txn, struct tuple *old_tuple; if (index_get(pk, key, part_count, &old_tuple) != 0) return -1; - if (old_tuple != NULL && - memtx_space->replace(space, old_tuple, NULL, - DUP_REPLACE_OR_INSERT, &stmt->old_tuple) != 0) - return -1; + if (old_tuple != NULL) { + memtx_engine_set_quota_strictness(space->engine, false); + int rc = memtx_space->replace(space, old_tuple, NULL, + DUP_REPLACE_OR_INSERT, + &stmt->old_tuple); + memtx_engine_set_quota_strictness(space->engine, true); + if (rc != 0) + return -1; + } stmt->engine_savepoint = stmt; *result = stmt->old_tuple; return 0; diff --git a/src/lib/small b/src/lib/small index 50cb78743..bdcd569f9 160000 --- a/src/lib/small +++ b/src/lib/small @@ -1 +1 @@ -Subproject commit 50cb7874380b286f3b34c82299cf5eb88b0d0059 +Subproject commit bdcd569f9ed753efb805f708089ca86e2520a44f -- 2.17.1 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-01-20 18:13 ` [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion Ilya Kosarev @ 2020-01-21 11:42 ` Nikita Pettik 2020-01-21 20:59 ` Vladislav Shpilevoy 2020-01-31 8:21 ` Konstantin Osipov 1 sibling, 1 reply; 27+ messages in thread From: Nikita Pettik @ 2020-01-21 11:42 UTC (permalink / raw) To: Ilya Kosarev; +Cc: tarantool-patches, v.shpilevoy On 20 Jan 21:13, Ilya Kosarev wrote: > diff --git a/src/box/box.cc b/src/box/box.cc > index 1b2b27d61..678c9ffe3 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -1321,9 +1321,14 @@ space_truncate(struct space *space) > ops_buf_end = mp_encode_uint(ops_buf_end, 1); > assert(ops_buf_end < buf + buf_size); > > + struct space *truncate_space = space_cache_find_xc(BOX_TRUNCATE_ID); > + memtx_engine_set_quota_strictness(truncate_space->engine, false); I wouldn't call it strictness. In fact you simply turn quota on/off. So I'd better call it quota_enable/disable. Nit: please accompany your code with (at least brief) comments. > + > if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end, > ops_buf, ops_buf_end, 0, NULL) != 0) > diag_raise(); If box_upsert() fails, quota is not re-enabled, isn't it? > + > + memtx_engine_set_quota_strictness(truncate_space->engine, true); > } > > int > diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c > index 4da80824a..c19205d34 100644 > --- a/src/box/memtx_engine.c > +++ b/src/box/memtx_engine.c > @@ -1090,6 +1090,15 @@ memtx_engine_set_memory(struct memtx_engine *memtx, size_t size) > return 0; > } > > +void > +memtx_engine_set_quota_strictness(struct engine *engine, bool strict) > +{ > + if (strncmp(engine->name, "memtx", 5) != 0) > + return; memtx_ prefix in the name of this function assumes that it is called only for memtx engine. Note that there's no such checks in memtx_engine interface functions. > + struct memtx_engine *memtx = (struct memtx_engine *)engine; > + quota_set_strictness(&memtx->quota, strict); What happens if quota is turned on back, but it still exceeds limit? I guess it would turn out to be in inconsistent state, since in normal operation mode there's no chance of being beyond the limit. > + > void > memtx_engine_set_max_tuple_size(struct memtx_engine *memtx, size_t max_size) > { > diff --git a/src/box/memtx_engine.h b/src/box/memtx_engine.h > index f562c66df..0793fc2ab 100644 > --- a/src/box/memtx_engine.h > +++ b/src/box/memtx_engine.h > @@ -213,6 +213,9 @@ memtx_engine_set_snap_io_rate_limit(struct memtx_engine *memtx, double limit); > int > memtx_engine_set_memory(struct memtx_engine *memtx, size_t size); > > +void > +memtx_engine_set_quota_strictness(struct engine *engine, bool strict); > + > void > memtx_engine_set_max_tuple_size(struct memtx_engine *memtx, size_t max_size); > > diff --git a/src/lib/small b/src/lib/small > index 50cb78743..bdcd569f9 160000 > --- a/src/lib/small > +++ b/src/lib/small What about adding unit and regression tests? > @@ -1 +1 @@ > -Subproject commit 50cb7874380b286f3b34c82299cf5eb88b0d0059 > +Subproject commit bdcd569f9ed753efb805f708089ca86e2520a44f > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-01-21 11:42 ` Nikita Pettik @ 2020-01-21 20:59 ` Vladislav Shpilevoy 2020-02-14 19:57 ` Ilya Kosarev 0 siblings, 1 reply; 27+ messages in thread From: Vladislav Shpilevoy @ 2020-01-21 20:59 UTC (permalink / raw) To: Nikita Pettik, Ilya Kosarev; +Cc: tarantool-patches Thanks for the patch! I answer some of Nikita's questions below and provide my own review further. See 7 comments below. On 21/01/2020 12:42, Nikita Pettik wrote: > On 20 Jan 21:13, Ilya Kosarev wrote: >> diff --git a/src/box/box.cc b/src/box/box.cc >> index 1b2b27d61..678c9ffe3 100644 >> --- a/src/box/box.cc >> +++ b/src/box/box.cc >> @@ -1321,9 +1321,14 @@ space_truncate(struct space *space) >> ops_buf_end = mp_encode_uint(ops_buf_end, 1); >> assert(ops_buf_end < buf + buf_size); >> >> + struct space *truncate_space = space_cache_find_xc(BOX_TRUNCATE_ID); >> + memtx_engine_set_quota_strictness(truncate_space->engine, false); > > I wouldn't call it strictness. In fact you simply turn quota on/off. > So I'd better call it quota_enable/disable. > > Nit: please accompany your code with (at least brief) comments. 1. On/off sounds good. >> + >> if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end, >> ops_buf, ops_buf_end, 0, NULL) != 0) >> diag_raise(); > > If box_upsert() fails, quota is not re-enabled, isn't it? 2. This is a bug, yes. But not the only one. The upsert will write to WAL and yield. During that yield the quota is turned off. And any other fiber can interfere and overuse the quota even more with something not related to delete/truncate. This can be fixed, for example, by making txn_begin() + quota_off() + upsert() + quota_on() + commit(). Also we could just not touch truncate and fix delete() only. Delete() can never leave the quota overused, because is not stored anywhere in memtx. While truncate is stored in _truncate. And delete would be sufficient, because can be used to free the quota enough so as to call truncate()/whatever else the user wanted. >> + >> + memtx_engine_set_quota_strictness(truncate_space->engine, true); >> } >> >> int >> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c >> index 4da80824a..c19205d34 100644 >> --- a/src/box/memtx_engine.c >> +++ b/src/box/memtx_engine.c >> @@ -1090,6 +1090,15 @@ memtx_engine_set_memory(struct memtx_engine *memtx, size_t size) >> return 0; >> } >> >> +void >> +memtx_engine_set_quota_strictness(struct engine *engine, bool strict) >> +{ >> + if (strncmp(engine->name, "memtx", 5) != 0) >> + return; > > memtx_ prefix in the name of this function assumes that it is called > only for memtx engine. Note that there's no such checks in memtx_engine > interface functions. > >> + struct memtx_engine *memtx = (struct memtx_engine *)engine; >> + quota_set_strictness(&memtx->quota, strict); > > What happens if quota is turned on back, but it still exceeds limit? > I guess it would turn out to be in inconsistent state, since in > normal operation mode there's no chance of being beyond the limit. 3. That problem is not solvable. There is always a chance, that truncate won't free any memory (for example, the target space could be empty already). If, after the quota is back on, it appears to be overused, nothing can be done with it. It will stay overused until more delete/truncate will be executed and actually delete something, or the user will increase memtx memory. > commit 45d9f491f91d93c8835ca725c234f7113c0d2aa8 > Author: Ilya Kosarev <i.kosarev@tarantool.org> > Date: Mon Jan 20 20:21:37 2020 +0300 > > memtx: allow quota overuse for truncation and deletion > > > 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 allowing > to overuse memtx quota if needed for truncation or deletion, using flag > in struct quota. After performing truncation or deletion we reset the > flag so that quota can be overused any more. > > Closes #3807 > > diff --git a/src/box/box.cc b/src/box/box.cc > index 1b2b27d61..678c9ffe3 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -1321,9 +1321,14 @@ space_truncate(struct space *space) > ops_buf_end = mp_encode_uint(ops_buf_end, 1); > assert(ops_buf_end < buf + buf_size); > > + struct space *truncate_space = space_cache_find_xc(BOX_TRUNCATE_ID); > + memtx_engine_set_quota_strictness(truncate_space->engine, false); > + > if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end, > ops_buf, ops_buf_end, 0, NULL) != 0) > diag_raise(); > + > + memtx_engine_set_quota_strictness(truncate_space->engine, true); > } 4. IMO, we should not touch quota at all until we see a not empty diag with OOM error type in it. Both here and in delete. Truncate is not a frequent operation (although it may be in some rare installations), but delete surely should not touch quota each time. > int > diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c > index 4da80824a..c19205d34 100644 > --- a/src/box/memtx_engine.c > +++ b/src/box/memtx_engine.c > @@ -1090,6 +1090,15 @@ memtx_engine_set_memory(struct memtx_engine *memtx, size_t size) > return 0; > } > > +void > +memtx_engine_set_quota_strictness(struct engine *engine, bool strict) > +{ > + if (strncmp(engine->name, "memtx", 5) != 0) > + return; 5. That check should not exist. It slows down the caller, and in upper code you always know whether you work with memtx or not. So it makes no sense to check it here. Truncate is always about insertion into a memtx space. Delete touches qouta inside memtx_space implementation. No chances that a vinyl space would call this function. > + struct memtx_engine *memtx = (struct memtx_engine *)engine; > + quota_set_strictness(&memtx->quota, strict); > +} > + > void > memtx_engine_set_max_tuple_size(struct memtx_engine *memtx, size_t max_size) > { > diff --git a/src/lib/small b/src/lib/small > index 50cb78743..bdcd569f9 160000 > --- a/src/lib/small > +++ b/src/lib/small > @@ -1 +1 @@ > -Subproject commit 50cb7874380b286f3b34c82299cf5eb88b0d0059 > +Subproject commit bdcd569f9ed753efb805f708089ca86e2520a44f 6. Please, write tests. They should include the bug test itself, and complex things such as quota overuse kept in case truncate() didn't free anything. You can use memtx_memory parameter to make it small enough so as the test would not take much time. 7. Seems like you changed something in small/. You need to send it as a separate patch with its own tests, and all. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-01-21 20:59 ` Vladislav Shpilevoy @ 2020-02-14 19:57 ` Ilya Kosarev 0 siblings, 0 replies; 27+ messages in thread From: Ilya Kosarev @ 2020-02-14 19:57 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 6617 bytes --] Thanks for your review! Sent tarantool/small patch and v4 of this patchset considering the drawbacks. >Вторник, 21 января 2020, 23:59 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>: > >Thanks for the patch! > >I answer some of Nikita's questions below and >provide my own review further. > >See 7 comments below. > >On 21/01/2020 12:42, Nikita Pettik wrote: >> On 20 Jan 21:13, Ilya Kosarev wrote: >>> diff --git a/src/box/box.cc b/src/box/box.cc >>> index 1b2b27d61..678c9ffe3 100644 >>> --- a/src/box/box.cc >>> +++ b/src/box/box.cc >>> @@ -1321,9 +1321,14 @@ space_truncate(struct space *space) >>> ops_buf_end = mp_encode_uint(ops_buf_end, 1); >>> assert(ops_buf_end < buf + buf_size); >>> >>> + struct space *truncate_space = space_cache_find_xc(BOX_TRUNCATE_ID); >>> + memtx_engine_set_quota_strictness(truncate_space->engine, false); >> >> I wouldn't call it strictness. In fact you simply turn quota on/off. >> So I'd better call it quota_enable/disable. >> >> Nit: please accompany your code with (at least brief) comments. > >1. On/off sounds good. > >>> + >>> if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end, >>> ops_buf, ops_buf_end, 0, NULL) != 0) >>> diag_raise(); >> >> If box_upsert() fails, quota is not re-enabled, isn't it? > >2. This is a bug, yes. But not the only one. The upsert will write >to WAL and yield. During that yield the quota is turned off. And >any other fiber can interfere and overuse the quota even more with >something not related to delete/truncate. > >This can be fixed, for example, by making txn_begin() + quota_off() + >upsert() + quota_on() + commit(). Also we could just not touch truncate >and fix delete() only. Delete() can never leave the quota overused, >because is not stored anywhere in memtx. While truncate is stored in >_truncate. And delete would be sufficient, because can be used to free >the quota enough so as to call truncate()/whatever else the user wanted. > >>> + >>> + memtx_engine_set_quota_strictness(truncate_space->engine, true); >>> } >>> >>> int >>> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c >>> index 4da80824a..c19205d34 100644 >>> --- a/src/box/memtx_engine.c >>> +++ b/src/box/memtx_engine.c >>> @@ -1090,6 +1090,15 @@ memtx_engine_set_memory(struct memtx_engine *memtx, size_t size) >>> return 0; >>> } >>> >>> +void >>> +memtx_engine_set_quota_strictness(struct engine *engine, bool strict) >>> +{ >>> + if (strncmp(engine->name, "memtx", 5) != 0) >>> + return; >> >> memtx_ prefix in the name of this function assumes that it is called >> only for memtx engine. Note that there's no such checks in memtx_engine >> interface functions. >> >>> + struct memtx_engine *memtx = (struct memtx_engine *)engine; >>> + quota_set_strictness(&memtx->quota, strict); >> >> What happens if quota is turned on back, but it still exceeds limit? >> I guess it would turn out to be in inconsistent state, since in >> normal operation mode there's no chance of being beyond the limit. > >3. That problem is not solvable. There is always a chance, that truncate >won't free any memory (for example, the target space could be empty >already). If, after the quota is back on, it appears to be overused, nothing >can be done with it. It will stay overused until more delete/truncate will be >executed and actually delete something, or the user will increase memtx memory. > >> commit 45d9f491f91d93c8835ca725c234f7113c0d2aa8 >> Author: Ilya Kosarev < i.kosarev@tarantool.org > >> Date: Mon Jan 20 20:21:37 2020 +0300 >> >> memtx: allow quota overuse for truncation and deletion >> >> >> 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 allowing >> to overuse memtx quota if needed for truncation or deletion, using flag >> in struct quota. After performing truncation or deletion we reset the >> flag so that quota can be overused any more. >> >> Closes #3807 >> >> diff --git a/src/box/box.cc b/src/box/box.cc >> index 1b2b27d61..678c9ffe3 100644 >> --- a/src/box/box.cc >> +++ b/src/box/box.cc >> @@ -1321,9 +1321,14 @@ space_truncate(struct space *space) >> ops_buf_end = mp_encode_uint(ops_buf_end, 1); >> assert(ops_buf_end < buf + buf_size); >> >> + struct space *truncate_space = space_cache_find_xc(BOX_TRUNCATE_ID); >> + memtx_engine_set_quota_strictness(truncate_space->engine, false); >> + >> if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end, >> ops_buf, ops_buf_end, 0, NULL) != 0) >> diag_raise(); >> + >> + memtx_engine_set_quota_strictness(truncate_space->engine, true); >> } > >4. IMO, we should not touch quota at all until we see a not empty diag >with OOM error type in it. Both here and in delete. Truncate is not >a frequent operation (although it may be in some rare installations), >but delete surely should not touch quota each time. > >> int >> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c >> index 4da80824a..c19205d34 100644 >> --- a/src/box/memtx_engine.c >> +++ b/src/box/memtx_engine.c >> @@ -1090,6 +1090,15 @@ memtx_engine_set_memory(struct memtx_engine *memtx, size_t size) >> return 0; >> } >> >> +void >> +memtx_engine_set_quota_strictness(struct engine *engine, bool strict) >> +{ >> + if (strncmp(engine->name, "memtx", 5) != 0) >> + return; > >5. That check should not exist. It slows down the caller, and >in upper code you always know whether you work with memtx or >not. So it makes no sense to check it here. Truncate is >always about insertion into a memtx space. Delete touches qouta >inside memtx_space implementation. No chances that a vinyl space >would call this function. > >> + struct memtx_engine *memtx = (struct memtx_engine *)engine; >> + quota_set_strictness(&memtx->quota, strict); >> +} >> + >> void >> memtx_engine_set_max_tuple_size(struct memtx_engine *memtx, size_t max_size) >> { >> diff --git a/src/lib/small b/src/lib/small >> index 50cb78743..bdcd569f9 160000 >> --- a/src/lib/small >> +++ b/src/lib/small >> @@ -1 +1 @@ >> -Subproject commit 50cb7874380b286f3b34c82299cf5eb88b0d0059 >> +Subproject commit bdcd569f9ed753efb805f708089ca86e2520a44f > >6. Please, write tests. They should include the bug test itself, and >complex things such as quota overuse kept in case truncate() didn't >free anything. You can use memtx_memory parameter to make it small >enough so as the test would not take much time. > >7. Seems like you changed something in small/. You need to send it as >a separate patch with its own tests, and all. -- Ilya Kosarev [-- Attachment #2: Type: text/html, Size: 7916 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-01-20 18:13 ` [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion Ilya Kosarev 2020-01-21 11:42 ` Nikita Pettik @ 2020-01-31 8:21 ` Konstantin Osipov 2020-02-04 18:56 ` Ilya Kosarev 1 sibling, 1 reply; 27+ messages in thread From: Konstantin Osipov @ 2020-01-31 8:21 UTC (permalink / raw) To: Ilya Kosarev; +Cc: tarantool-patches, v.shpilevoy * Ilya Kosarev <i.kosarev@tarantool.org> [20/01/20 21:18]: > 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 allowing > to overuse memtx quota if needed for truncation or deletion, using flag > in struct quota. After performing truncation or deletion we reset the > flag so that quota can be overused any more. This is duplicating the tech already existing with reserved extents. Why not use the reserved extents instead? Why do you need to turn quota completely on/off? -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-01-31 8:21 ` Konstantin Osipov @ 2020-02-04 18:56 ` Ilya Kosarev 2020-02-04 20:06 ` Konstantin Osipov 0 siblings, 1 reply; 27+ messages in thread From: Ilya Kosarev @ 2020-02-04 18:56 UTC (permalink / raw) To: Konstantin Osipov; +Cc: tarantool-patches, v.shpilevoy [-- Attachment #1: Type: text/plain, Size: 959 bytes --] Hi! Thanks for your comments. When performing space: truncate(), we are executing simple upsert. How can we distinguish it from any other upsert to use reserved extents tech? пятница, 31 января 2020г., 11:21 +03:00 от Konstantin Osipov kostja.osipov@gmail.com : >* Ilya Kosarev < i.kosarev@tarantool.org> [20/01/20 21:18]: > 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 allowing > to overuse memtx quota if needed for truncation or deletion, using flag > in struct quota. After performing truncation or deletion we reset the > flag so that quota can be overused any more. > >This is duplicating the tech already existing with reserved >extents. > >Why not use the reserved extents instead? Why do you need to turn >quota completely on/off? > > >-- >Konstantin Osipov, Moscow, Russia [-- Attachment #2: Type: text/html, Size: 1721 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-02-04 18:56 ` Ilya Kosarev @ 2020-02-04 20:06 ` Konstantin Osipov 2020-02-05 19:11 ` Ilya Kosarev 0 siblings, 1 reply; 27+ messages in thread From: Konstantin Osipov @ 2020-02-04 20:06 UTC (permalink / raw) To: Ilya Kosarev; +Cc: tarantool-patches, v.shpilevoy * Ilya Kosarev <i.kosarev@tarantool.org> [20/02/04 22:00]: > Hi! > Thanks for your comments. > When performing space: truncate(), we are executing > simple upsert. How can we distinguish it from any other upsert > to use reserved extents tech? пятница, 31 января 2020г., 11:21 What code path is troubling you? Just like you disable quota, you can "enable" use of reserved extents. You can use the same injection points for this code, the only difference is that you don't allocate memory beyond quota, but rather use pre-allocated one, within the quota. -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-02-04 20:06 ` Konstantin Osipov @ 2020-02-05 19:11 ` Ilya Kosarev 2020-02-05 19:17 ` Konstantin Osipov 0 siblings, 1 reply; 27+ messages in thread From: Ilya Kosarev @ 2020-02-05 19:11 UTC (permalink / raw) To: Konstantin Osipov; +Cc: tarantool-patches, v.shpilevoy [-- Attachment #1: Type: text/plain, Size: 899 bytes --] You mean using some specific extent_pool, like indext_extent_pool with it's memtx_index_extent_reserve() and memtx_index_extent_alloc()? >Вторник, 4 февраля 2020, 23:06 +03:00 от Konstantin Osipov <kostja.osipov@gmail.com>: > >* Ilya Kosarev < i.kosarev@tarantool.org > [20/02/04 22:00]: >> Hi! >> Thanks for your comments. >> When performing space: truncate(), we are executing >> simple upsert. How can we distinguish it from any other upsert >> to use reserved extents tech? пятница, 31 января 2020г., 11:21 >What code path is troubling you? > >Just like you disable quota, you can "enable" use of reserved >extents. You can use the same injection points for this code, the >only difference is that you don't allocate memory beyond quota, >but rather use pre-allocated one, within the quota. > > >-- >Konstantin Osipov, Moscow, Russia -- Ilya Kosarev [-- Attachment #2: Type: text/html, Size: 1486 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-02-05 19:11 ` Ilya Kosarev @ 2020-02-05 19:17 ` Konstantin Osipov 2020-02-06 10:50 ` Ilya Kosarev 0 siblings, 1 reply; 27+ messages in thread From: Konstantin Osipov @ 2020-02-05 19:17 UTC (permalink / raw) To: Ilya Kosarev; +Cc: tarantool-patches, v.shpilevoy * Ilya Kosarev <i.kosarev@tarantool.org> [20/02/05 22:15]: > > You mean using some specific extent_pool, like indext_extent_pool > with it's memtx_index_extent_reserve() and memtx_index_extent_alloc()? Yes. -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-02-05 19:17 ` Konstantin Osipov @ 2020-02-06 10:50 ` Ilya Kosarev 2020-02-06 14:29 ` Konstantin Osipov 0 siblings, 1 reply; 27+ messages in thread From: Ilya Kosarev @ 2020-02-06 10:50 UTC (permalink / raw) To: Konstantin Osipov; +Cc: tarantool-patches, v.shpilevoy [-- Attachment #1: Type: text/plain, Size: 692 bytes --] Well, the problem here is that garbage collection won't work properly in this case. I guess it can be adjusted, however, there will be some kind of crutches and it doesn't seem to be a good idea. Just switching quota on and off seems to be more suitable and convenient for truncations and deletions. >Среда, 5 февраля 2020, 22:17 +03:00 от Konstantin Osipov <kostja.osipov@gmail.com>: > >* Ilya Kosarev < i.kosarev@tarantool.org > [20/02/05 22:15]: >> >> You mean using some specific extent_pool, like indext_extent_pool >> with it's memtx_index_extent_reserve() and memtx_index_extent_alloc()? >Yes. > > > >-- >Konstantin Osipov, Moscow, Russia -- Ilya Kosarev [-- Attachment #2: Type: text/html, Size: 1251 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-02-06 10:50 ` Ilya Kosarev @ 2020-02-06 14:29 ` Konstantin Osipov 2020-02-06 16:14 ` Ilya Kosarev 0 siblings, 1 reply; 27+ messages in thread From: Konstantin Osipov @ 2020-02-06 14:29 UTC (permalink / raw) To: Ilya Kosarev; +Cc: tarantool-patches, v.shpilevoy * Ilya Kosarev <i.kosarev@tarantool.org> [20/02/06 13:51]: > > Well, the problem here is that garbage collection won't work properly > in this case. I don't understand what GC has to do with it. Please clarify. > I guess it can be adjusted, however, there will be some > kind of crutches and it doesn't seem to be a good idea. Just switching > quota on and off seems to be more suitable and convenient for > truncations and deletions. > -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-02-06 14:29 ` Konstantin Osipov @ 2020-02-06 16:14 ` Ilya Kosarev 2020-02-06 16:26 ` Konstantin Osipov 0 siblings, 1 reply; 27+ messages in thread From: Ilya Kosarev @ 2020-02-06 16:14 UTC (permalink / raw) To: Konstantin Osipov; +Cc: tarantool-patches, v.shpilevoy [-- Attachment #1: Type: text/plain, Size: 1125 bytes --] From memtx_tuple_new() we call smalloc(). In case it is calling mempool_alloc(), we are passing the pool from memtx->alloc, chosen depending on it's size. Later this tuple may be deleted. Then smfree() is called, which uses mempool_find() to acquire the pool we want to free. In case we are using some other specific pool, current mempool_find() won't be able to find it. This means we need to process this case explicitly and it seems to add noticeable unjustified complexity to the code. >Четверг, 6 февраля 2020, 17:29 +03:00 от Konstantin Osipov <kostja.osipov@gmail.com>: > >* Ilya Kosarev < i.kosarev@tarantool.org > [20/02/06 13:51]: >> >> Well, the problem here is that garbage collection won't work properly >> in this case. > >I don't understand what GC has to do with it. Please clarify. > >> I guess it can be adjusted, however, there will be some >> kind of crutches and it doesn't seem to be a good idea. Just switching >> quota on and off seems to be more suitable and convenient for >> truncations and deletions. >> > >-- >Konstantin Osipov, Moscow, Russia -- Ilya Kosarev [-- Attachment #2: Type: text/html, Size: 1696 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-02-06 16:14 ` Ilya Kosarev @ 2020-02-06 16:26 ` Konstantin Osipov 2020-02-10 8:24 ` Ilya Kosarev 0 siblings, 1 reply; 27+ messages in thread From: Konstantin Osipov @ 2020-02-06 16:26 UTC (permalink / raw) To: Ilya Kosarev; +Cc: tarantool-patches, v.shpilevoy * Ilya Kosarev <i.kosarev@tarantool.org> [20/02/06 19:15]: > > From memtx_tuple_new() we call smalloc(). In case it is calling > mempool_alloc(), we are passing the pool from memtx->alloc, chosen > depending on it's size. Later this tuple may be deleted. Then smfree() > is called, which uses mempool_find() to acquire the pool we want to > free. > In case we are using some other specific pool, current mempool_find() > won't be able to find it. This means we need to process this case > explicitly and it seems to add noticeable unjustified complexity to the > code. Why do you need to use a special pool? Can't you reserve a single slab in memtx_arena, just like memtx reserves extents now, and allow use of this slab for any pool *only* when processing truncate? -- Konstantin Osipov, Moscow, Russia https://scylladb.com ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-02-06 16:26 ` Konstantin Osipov @ 2020-02-10 8:24 ` Ilya Kosarev 2020-02-10 9:49 ` Konstantin Osipov 0 siblings, 1 reply; 27+ messages in thread From: Ilya Kosarev @ 2020-02-10 8:24 UTC (permalink / raw) To: Konstantin Osipov; +Cc: tarantool-patches, v.shpilevoy [-- Attachment #1: Type: text/plain, Size: 1662 bytes --] As far as i see, memtx reserves extents using specific pool and mempool_alloc. We will need to implement additional feature to reserve slabs in memtx arena and provide them specifically for truncation needs. However, after that we will encounter the necessity to calculate how much space we need to reserve for truncation tuples. I guess we might want to reserve a truncation buf for each created space and re-reserve it on each truncation, although such logic seems overcomplicated. Furthermore, the problem with space:delete() might happen exactly when we are trying to reserve a slab to guarantee successful statement-level rollback. This can't be fixed using reservation tech. >Четверг, 6 февраля 2020, 19:26 +03:00 от Konstantin Osipov <kostja.osipov@gmail.com>: > >* Ilya Kosarev < i.kosarev@tarantool.org > [20/02/06 19:15]: >> >> From memtx_tuple_new() we call smalloc(). In case it is calling >> mempool_alloc(), we are passing the pool from memtx->alloc, chosen >> depending on it's size. Later this tuple may be deleted. Then smfree() >> is called, which uses mempool_find() to acquire the pool we want to >> free. >> In case we are using some other specific pool, current mempool_find() >> won't be able to find it. This means we need to process this case >> explicitly and it seems to add noticeable unjustified complexity to the >> code. >Why do you need to use a special pool? Can't you reserve a single >slab in memtx_arena, just like memtx reserves extents now, and >allow use of this slab for any pool *only* when processing truncate? > >-- >Konstantin Osipov, Moscow, Russia >https://scylladb.com -- Ilya Kosarev [-- Attachment #2: Type: text/html, Size: 2338 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-02-10 8:24 ` Ilya Kosarev @ 2020-02-10 9:49 ` Konstantin Osipov 2020-02-10 10:43 ` Ilya Kosarev 0 siblings, 1 reply; 27+ messages in thread From: Konstantin Osipov @ 2020-02-10 9:49 UTC (permalink / raw) To: Ilya Kosarev; +Cc: tarantool-patches, v.shpilevoy * Ilya Kosarev <i.kosarev@tarantool.org> [20/02/10 11:26]: > > As far as i see, memtx reserves extents using specific pool and > mempool_alloc. We will need to implement additional feature to reserve > slabs in memtx arena and provide them specifically for truncation > needs. Yes. But the way you say it sounds negative. Are you suggesting not implementing this feature and just hacking in quota bypass is better? Why? > However, after that we will encounter the necessity to calculate > how much space we need to reserve for truncation tuples. No. 1 slab is enough. > I guess we > might want to reserve a truncation buf for each created space and > re-reserve it on each truncation, although such logic seems > overcomplicated. Indeed, this would be strange. > Furthermore, the problem with space:delete() might happen exactly when > we are trying to reserve a slab to guarantee successful statement-level > rollback. This can't be fixed using reservation tech. Two events can't happen at the same time in a single-threaded application. Please take time to explain what you mean here. -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-02-10 9:49 ` Konstantin Osipov @ 2020-02-10 10:43 ` Ilya Kosarev 2020-02-10 10:50 ` Konstantin Osipov 0 siblings, 1 reply; 27+ messages in thread From: Ilya Kosarev @ 2020-02-10 10:43 UTC (permalink / raw) To: Konstantin Osipov; +Cc: tarantool-patches, v.shpilevoy [-- Attachment #1: Type: text/plain, Size: 1801 bytes --] >> As far as i see, memtx reserves extents using specific pool and >> mempool_alloc. We will need to implement additional feature to reserve >> slabs in memtx arena and provide them specifically for truncation >> needs. > >Yes. But the way you say it sounds negative. Are you suggesting >not implementing this feature and just hacking in quota bypass is >better? Why? I am afraid that ‘reserving’ approach both doesn’t always guarantee successful truncation (without some overcomplicated actions) and adds extra code complexity comparing to the quota on/off mechansm. I see that simple quota hacking doesn’t look good, but i think it might be even worse to implement partial solution along with all the extra code. So far it might be better not to touch this problem, as Vlad suggests, considering it as not critical. >> However, after that we will encounter the necessity to calculate >> how much space we need to reserve for truncation tuples. > >No. 1 slab is enough. > >> I guess we >> might want to reserve a truncation buf for each created space and >> re-reserve it on each truncation, although such logic seems >> overcomplicated. > >Indeed, this would be strange. > >> Furthermore, the problem with space:delete() might happen exactly when >> we are trying to reserve a slab to guarantee successful statement-level >> rollback. This can't be fixed using reservation tech. > >Two events can't happen at the same time in a single-threaded >application. Please take time to explain what you mean here. I mean «when» in terms of place (where we reserve), not time. I mean this is one more problem which can't be solved using 'reserve' tech, although i am not sure it really should be solved through quota bypass. >-- >Konstantin Osipov, Moscow, Russia -- Ilya Kosarev [-- Attachment #2: Type: text/html, Size: 2428 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-02-10 10:43 ` Ilya Kosarev @ 2020-02-10 10:50 ` Konstantin Osipov 2020-02-14 19:55 ` Ilya Kosarev 0 siblings, 1 reply; 27+ messages in thread From: Konstantin Osipov @ 2020-02-10 10:50 UTC (permalink / raw) To: Ilya Kosarev; +Cc: tarantool-patches, v.shpilevoy * Ilya Kosarev <i.kosarev@tarantool.org> [20/02/10 13:44]: Ilya, please explain the issues in specific terms: references to source code, test conditions, and so on. I don't see any issue with reserving a slab at Tarantool start for out-of-memory truncate. I don't see any issues with deletes, because deletes are not supposed to use this slab, it should only hold truncate record. Deletes do not allocate tuples anyway. I think you should try to do it and send a patch when you get stuck. You can't expect me to explain how to do it because I don't know what you don't know. -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion 2020-02-10 10:50 ` Konstantin Osipov @ 2020-02-14 19:55 ` Ilya Kosarev 0 siblings, 0 replies; 27+ messages in thread From: Ilya Kosarev @ 2020-02-14 19:55 UTC (permalink / raw) To: Konstantin Osipov; +Cc: tarantool-patches, v.shpilevoy [-- Attachment #1: Type: text/plain, Size: 1061 bytes --] Sent tarantool/small patch and patchset with explanations on reserve tech drawbacks and space:delete() problem: https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/014191.html >Понедельник, 10 февраля 2020, 13:50 +03:00 от Konstantin Osipov <kostja.osipov@gmail.com>: > >* Ilya Kosarev < i.kosarev@tarantool.org > [20/02/10 13:44]: > >Ilya, please explain the issues in specific terms: references to >source code, test conditions, and so on. > >I don't see any issue with reserving a slab at Tarantool start for >out-of-memory truncate. I don't see any issues with deletes, >because deletes are not supposed to use this slab, it should only hold >truncate record. Deletes do not allocate tuples anyway. Right, space:delete() problem is not connected with the space:truncate() problem. > >I think you should try to do it and send a patch when you get >stuck. > >You can't expect me to explain how to do it because I don't know >what you don't know. > >-- >Konstantin Osipov, Moscow, Russia -- Ilya Kosarev [-- Attachment #2: Type: text/html, Size: 1795 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2020-02-14 19:57 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-20 18:13 [Tarantool-patches] [PATCH v3 0/2] Safe truncation and deletion Ilya Kosarev 2020-01-20 18:13 ` [Tarantool-patches] [PATCH v3 1/2] b-tree: return NULL on matras_alloc fail Ilya Kosarev 2020-01-21 10:32 ` Nikita Pettik 2020-01-31 8:18 ` Konstantin Osipov 2020-02-04 17:13 ` Nikita Pettik 2020-02-04 17:25 ` Konstantin Osipov 2020-02-04 18:08 ` Nikita Pettik 2020-02-04 18:25 ` Konstantin Osipov 2020-01-21 20:55 ` Vladislav Shpilevoy 2020-01-20 18:13 ` [Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion Ilya Kosarev 2020-01-21 11:42 ` Nikita Pettik 2020-01-21 20:59 ` Vladislav Shpilevoy 2020-02-14 19:57 ` Ilya Kosarev 2020-01-31 8:21 ` Konstantin Osipov 2020-02-04 18:56 ` Ilya Kosarev 2020-02-04 20:06 ` Konstantin Osipov 2020-02-05 19:11 ` Ilya Kosarev 2020-02-05 19:17 ` Konstantin Osipov 2020-02-06 10:50 ` Ilya Kosarev 2020-02-06 14:29 ` Konstantin Osipov 2020-02-06 16:14 ` Ilya Kosarev 2020-02-06 16:26 ` Konstantin Osipov 2020-02-10 8:24 ` Ilya Kosarev 2020-02-10 9:49 ` Konstantin Osipov 2020-02-10 10:43 ` Ilya Kosarev 2020-02-10 10:50 ` Konstantin Osipov 2020-02-14 19:55 ` Ilya Kosarev
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox