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

[Olga Arkhangelskaia <arkholga@tarantool.org>]

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.

>