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

[Olga Arkhangelskaia <arkholga@tarantool.org>]

On 24/12/2019 11:06, Olga Arkhangelskaia wrote:
> Hi Nikita, thanks for review.
> On 20/12/2019 18:35, Nikita Pettik wrote:
>> On 20 Dec 16:44, Olga Arkhangelskaia wrote:
>>
>> Hello. Below are a few nits concerning code itself.
>>
>>> diff --git a/src/box/index.cc b/src/box/index.cc
>>> index 4e4867118..d0aa62d67 100644
>>> --- a/src/box/index.cc
>>> +++ b/src/box/index.cc
>>> @@ -733,7 +733,8 @@ int
>>>   generic_index_build_next(struct index *index, struct tuple *tuple)
>>>   {
>>>       struct tuple *unused;
>>> -    return index_replace(index, NULL, tuple, DUP_INSERT, &unused);
>>> +    int res = index_reserve(index, 0);
>>> +    return res == 0 ? index_replace(index, NULL, tuple, DUP_INSERT, 
>>> &unused) : res;
>>>   }
>> Why not:
>>
>> if (index_reserve(index, 0) != 0)
>>     return -1;
>> return index_replace(...);
> Ok, I will change. I guess it did't come on my mind.
>>
>>>     void
>>> diff --git a/src/box/memtx_engine.h b/src/box/memtx_engine.h
>>> index f562c66df..f6335cc66 100644
>>> --- a/src/box/memtx_engine.h
>>> +++ b/src/box/memtx_engine.h
>>> @@ -87,6 +87,18 @@ enum memtx_recovery_state {
>>>   /** Memtx extents pool, available to statistics. */
>>>   extern struct mempool memtx_index_extent_pool;
>>>   +enum memtx_reserve_extents_num{
>>                                   ^ missed space
>>
>>> +    /**
>>> +     * This number is calculated based on the
>>> +     * max (realistic) number of insertions
>>> +     * a deletion from a B-tree or an R-tree
>>> +     * can lead to, and, as a result, the max
>>> +     * number of new block allocations.
>>> +     */
>>> +    RESERVE_EXTENTS_BEFORE_DELETE = 8,
>>> +    RESERVE_EXTENTS_BEFORE_REPLACE = 16
>>> +};
>>> +
>>>   /**
>>>    * The size of the biggest memtx iterator. Used with
>>>    * mempool_create. This is the size of the block that will be
>>> diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
>>> index 8badad797..805fdf54c 100644
>>> --- a/src/box/memtx_rtree.c
>>> +++ b/src/box/memtx_rtree.c
>>> @@ -242,6 +242,19 @@ memtx_rtree_index_replace(struct index *base, 
>>> struct tuple *old_tuple,
>>>       return 0;
>>>   }
>>>   +static int memtx_rtree_index_reserve(struct index *base, uint32_t 
>>> size_hint)
>> static int
>> memtx_rtree_index_reserve(...)
>>
>>> +{
>>> +    /**
>> Comments inside functions start from /* and should be restricted
>> with 66 characters.
> Will fix, and all above too.
>>
>>> +     * 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. It is just a check not index 
>>> reservation.
>>> +     */
>>> +    (void)size_hint;
>>> +    struct memtx_engine *memtx = (struct memtx_engine *)base->engine;
>>> +    return memtx_index_extent_reserve(memtx,
>>> + RESERVE_EXTENTS_BEFORE_REPLACE);
>> Looks like broken indentation.
>
> It is likely, I have change my work machine and some options may have 
> been missed, will pay more attention.
>
>
>>> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
>>> index 6ef84e045..da20b9196 100644
>>> --- a/src/box/memtx_space.c
>>> +++ b/src/box/memtx_space.c
>>> @@ -103,18 +103,6 @@ memtx_space_replace_no_keys(struct space 
>>> *space, struct tuple *old_tuple,
>>>       return -1;
>>>   }
>>>   -enum {
>>> -    /**
>>> -     * This number is calculated based on the
>>> -     * max (realistic) number of insertions
>>> -     * a deletion from a B-tree or an R-tree
>>> -     * can lead to, and, as a result, the max
>>> -     * number of new block allocations.
>>> -     */
>>> -    RESERVE_EXTENTS_BEFORE_DELETE = 8,
>>> -    RESERVE_EXTENTS_BEFORE_REPLACE = 16
>>> -};
>>> -
>>>   /**
>>>    * A short-cut version of replace() used during bulk load
>>>    * from snapshot.
>> I don't dive into reasons why you avoid using error injection
>> in this case, but still we have separate 'long_run' tests
>> (you can specify long_run option in suite.ini config).
>
> Actually it is long on my mac =)) However, I am not sure what to use
>
> 84     _(ERRINJ_INDEX_ALLOC, ERRINJ_BOOL
> 85     _(ERRINJ_TUPLE_ALLOC, ERRINJ_BOOL
>
> because it is extent allocation that fail. Will be glad to hear what 
> should be chosen.
>
I have looked closer - it is _(ERRINJ_INDEX_ALLOC, ERRINJ_BOOL actually.
>
>