Re: [Tarantool-patches] [PATCH v6] memtx: fix out of memory handling for rtree

[Olga Arkhangelskaia <arkholga@tarantool.org>]

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")