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

[Nikita Pettik <korablev@tarantool.org>]

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(...);

>  
>  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.

> +	 * 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.

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