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

[Nikita Pettik <korablev@tarantool.org>]

On 10 Feb 10:47, Olga Arkhangelskaia wrote:
> Changes in v2:
> - changed way of error handling, now we reserve pages before go to rtree lib
> - changed test
> - changed commit msg
> 
> Changes in v3:
> - added memtx_rtree_build_next function
> - memory reservation is moved to memtx_rtree_build_next
> 
> Changes in v4:
> - added index reservation in build_next
> - added memtx_rtree_reserve implementation
> 
> Changes in v5:
> - added error injection
> - test is placed in errorinj.test.lua
> - test is much faster now
> - fixed indentation errors 
> 
> Changes in v6:
> - fixed indentation
> - fixed test output

Bump patch version only in case functional changes are significant
enough. 

Please, do not send new patch in response to the current review,
simply inline your answers like "applied/skipped" etc.

From technical point of view patch LGTM. A few suggestions concerning
code documentation below (skip them or apply - it is up to you).
I guess smb else should look at the patch before it is pushed to
master (as a rule 2 LGTMs are enough).

> diff --git a/src/box/index.cc b/src/box/index.cc
> index 4e4867118..750838bef 100644
> --- a/src/box/index.cc
> +++ b/src/box/index.cc
> @@ -733,6 +733,8 @@ int
>  generic_index_build_next(struct index *index, struct tuple *tuple)
>  {
>  	struct tuple *unused;

I'd add a brief comment here:

diff --git a/src/box/index.cc b/src/box/index.cc
index 750838bef..c2fc00867 100644
--- a/src/box/index.cc
+++ b/src/box/index.cc
@@ -733,6 +733,11 @@ 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;

> +	if (index_reserve(index, 0) != 0)
> +		return -1;
>  	return index_replace(index, NULL, tuple, DUP_INSERT, &unused);
>  }
>  
> --- a/src/box/memtx_rtree.c
> +++ b/src/box/memtx_rtree.c
> @@ -242,6 +242,23 @@ 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)
> +{
> +	/*
> +	 * 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, "slab allocator", "memtx extent");
> +		return -1;
> +	});

Consider this small fix:

diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
index a5cae8f45..590321a7e 100644
--- a/src/box/memtx_rtree.c
+++ b/src/box/memtx_rtree.c
@@ -246,13 +246,14 @@ static int
 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 durig any operationin rtree, because there is no
-        * error handling in rtree lib.
+        * 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.
         */
        (void)size_hint;
        ERROR_INJECT(ERRINJ_INDEX_RESERVE, {
-               diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "slab allocator", "memtx extent");
+               diag_set(OutOfMemory, MEMTX_EXTENT_SIZE,"mempool", "new slab");
                return -1;
        });

> diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
> index 03c088677..598e7da33 100644
> --- a/test/box/errinj.test.lua
> +++ b/test/box/errinj.test.lua
> @@ -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-RTREE-doesn't-handle-OOM-properly
> +--

I'd a bit extend this comment like:

-- gh-4619: make sure that if OOM takes place during rtree recovery,
-- Tarantool instance will fail gracefully.

> +test_run:cmd('create server rtree with script = "box/lua/cfg_rtree.lua"')
> +test_run:cmd("start server rtree")
> +test_run:cmd('switch rtree')
> +math = require("math")