[Tarantool-patches] [PATCH v6] memtx: fix out of memory handling for rtree
Olga Arkhangelskaia
arkholga at tarantool.org
Tue Mar 3 14:50:17 MSK 2020
Hi Sergey! I need time to check everything!
I can not use error injection that exists in memtx_index_extent_reserve(),
because it is used in memtx_space_replace_all_keys ant it is called
prior memtx_rtree_index_reserve() .
If we use memtx_index_extent_reserve(), injection we will not reach the
place where error with rtree occures.
So to make sure that we fixed the problem we need
memtx_rtree_index_reserve() to fail.
19.02.2020 19:28, Sergey Ostanevich пишет:
> Hi!
>
> I am sorry, I took the OKriw/gh-4619-RTREE-doesnt-handle-OOM-properly
> branch for review, instead of OKriw/gh-RTREE-doesnt-handle-OOM-properly
>
> I can see memtx_index_extent_reserve() has an error injection already.
> Is it not sufficient, so you still need a new one in
> memtx_rtree_index_reserve() ?
> Otherwise looks good.
>
> Regards,
> Sergos
>
>
> On 18 фев 21:20, Olga Arkhangelskaia wrote:
>> Hi! Thanks for the review! But I am a bit confused.
>>
>> 18.02.2020 15:46, Sergey Ostanevich пишет:
>>> Hi!
>>>
>>> Thanks for the patch!
>>>
>>> I wonder why do you use additional by-ref argument to pass an integer
>>> result instead of redefining the interface into:
>>>
>>> int
>>> rtree_insert(struct rtree *tree, struct rtree_rect *rect, record_t obj)
>>>
>>> instead, so that all uses of a form:
>>>
>>> int err = 0;
>>> ...
>>> rtree_insert(&index->tree, &rect, new_tuple, &err);
>>
>> There is no way rtree returns an error. Library itself can not fail. And
>> the best way is to reserve pages before malloc itself in the library.
>>
>> We have discussed in the very beginning with Konstantin Osipov and later
>> with Aleksandr Lyapunov.
>>
>> https://lists.tarantool.org/pipermail/tarantool-patches/2019-November/012392.html
>>
>> So we reserve memory instead of returning an error.
>>
>>> if (err == 1) {
>>> diag_set(OutOfMemory, sizeof(struct rtree),
>>> "rtree", "rtree_page");
>>> return -1;
>>> }
>>>
>>> into a much simpler:
>>>
>>> if (rtree_insert(&index->tree, &rect, new_tuple, &err)) {
>>> diag_set(OutOfMemory, sizeof(struct rtree),
>>> "rtree", "rtree_page");
>>> return -1;
>>> }
>>>
>>> Sergos
>>>
>>> On 11 Feb 15:42, Nikita Pettik wrote:
>>>> On 11 Feb 13:51, Olga Arkhangelskaia wrote:
>>>>> Hi Nikita! Thanks for the review.
>>>>>
>>>>> I have fixed everything you have pointed out. Diff can be found below.
>>>> LGTM
>>>>> 10.02.2020 14:18, Nikita Pettik пишет:
>>>>>> On 10 Feb 10:47, Olga Arkhangelskaia wrote:
>>>>> diff --git a/src/box/index.cc b/src/box/index.cc
>>>>> -index 4e4867118..c2fc00867 100644
>>>>> +index 4e4867118..750838bef 100644
>>>>> --- a/src/box/index.cc
>>>>> +++ b/src/box/index.cc
>>>>> -@@ -733,6 +733,13 @@ int
>>>>> +@@ -733,6 +733,8 @@ int
>>>>> generic_index_build_next(struct index *index, struct tuple *tuple)
>>>>> {
>>>>> struct tuple *unused;
>>>>> -+ /*
>>>>> -+ * Note this is not no-op call in case of rtee index:
>>>>> -+ * reserving 0 bytes is required during rtree recovery.
>>>>> -+ * For details see memtx_rtree_index_reserve().
>>>>> -+ */
>>>>> + if (index_reserve(index, 0) != 0)
>>>>> + return -1;
>>>>>
>>>>> diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
>>>>> -index 8badad797..612fcb2a9 100644
>>>>> +index 8badad797..a5cae8f45 100644
>>>>> --- a/src/box/memtx_rtree.c
>>>>> +++ b/src/box/memtx_rtree.c
>>>>> -@@ -242,6 +242,24 @@ memtx_rtree_index_replace(struct index *base, struct
>>>>> tuple *old_tuple,
>>>>> +@@ -242,6 +242,23 @@ memtx_rtree_index_replace(struct index *base, struct
>>>>> tuple *old_tuple,
>>>>> return 0;
>>>>> }
>>>>>
>>>>> @@ -82,14 +109,13 @@
>>>>> +memtx_rtree_index_reserve(struct index *base, uint32_t size_hint)
>>>>> +{
>>>>> + /*
>>>>> -+ * In case of rtree we use reserve to make sure that
>>>>> -+ * memory allocation will not fail during any operation
>>>>> -+ * on rtree, because there is no error handling in the
>>>>> -+ * rtree lib.
>>>>> -+ */
>>>>> ++ * In case of rtree we use reserve to make sure that memory
>>>>> allocation
>>>>> ++ * will not fail durig any operationin rtree, because there is no
>>>>> ++ * error handling in rtree lib.
>>>>> ++ */
>>>>> + (void)size_hint;
>>>>> + ERROR_INJECT(ERRINJ_INDEX_RESERVE, {
>>>>> -+ diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "mempool", "new
>>>>> slab");
>>>>> ++ diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "slab allocator",
>>>>> "memtx extent");
>>>>> + return -1;
>>>>> + });
>>>>> + struct memtx_engine *memtx = (struct memtx_engine *)base->engine;
>>>>>
>>>>> diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
>>>>> -index 03c088677..63c8b19fe 100644
>>>>> +index 03c088677..598e7da33 100644
>>>>> --- a/test/box/errinj.test.lua
>>>>> +++ b/test/box/errinj.test.lua
>>>>> -@@ -620,3 +620,31 @@ box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0.5)
>>>>> +@@ -620,3 +620,30 @@ box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0.5)
>>>>> box.error.injection.get('ERRINJ_RELAY_TIMEOUT')
>>>>> box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0)
>>>>> box.error.injection.get('ERRINJ_RELAY_TIMEOUT')
>>>>> +
>>>>> +--
>>>>> -+-- gh-4619: make sure that if OOM takes place during rtree recovery,
>>>>> -+-- Tarantool instance will fail gracefully.
>>>>> ++-- gh-4619-RTREE-doesn't-handle-OOM-properly
>>>>> +--
>>>>> +test_run:cmd('create server rtree with script = "box/lua/cfg_rtree.lua"')
>>>>> +test_run:cmd("start server rtree")
More information about the Tarantool-patches
mailing list