[Tarantool-patches] [PATCH v4] memtx: fix out of memory handling for rtree
Olga Arkhangelskaia
arkholga at tarantool.org
Tue Dec 24 11:06:03 MSK 2019
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.
>
More information about the Tarantool-patches
mailing list