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

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

[Olga Arkhangelskaia]

When tarantool tries to recover rtree from a snapshot and memtx_memory value
is lower than it has been when the snapshot was created, server suffers from
segmentation fault. This happens because there is no out of memory error
handling in rtree lib. In another words, we do not check the result of
malloc operation.
The execution flow in case of recovery uses different way and the secondary
keys are build in batches. That way has no checks and reservations.
The patch adds memtx_rtree_index_reserve implementation to make sure that any
memory allocation in rtree will fail. Although this gives us no additional
optimization as in case of memtx_tree, the memory reservation prevents
tarantool from segmentation fault. If there is not enough memory to be reserved
server will fail gently with the "Failed to allocate" error message.

Closes #4619
---
Branch: https://github.com/tarantool/tarantool/tree/OKriw/gh-RTREE-doesnt-handle-OOM-properly
Issue:https://github.com/tarantool/tarantool/issues/4619

v1:https://lists.tarantool.org/pipermail/tarantool-patches/2019-November/012391.html
v2:https://lists.tarantool.org/pipermail/tarantool-patches/2019-December/012958.html
v3:https://lists.tarantool.org/pipermail/tarantool-patches/2019-December/013182.html
v4:https://lists.tarantool.org/pipermail/tarantool-patches/2019-December/013230.html
v5:https://lists.tarantool.org/pipermail/tarantool-patches/2019-December/013340.html

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
 
 src/box/index.cc           |   2 +
 src/box/memtx_engine.h     |  12 ++
 src/box/memtx_rtree.c      |  19 ++-
 src/box/memtx_space.c      |  12 --
 src/lib/core/errinj.h      |   1 +
 test/box/errinj.result     | 231 ++++++++++++++++++++++++++-----------
 test/box/errinj.test.lua   |  27 +++++
 test/box/lua/cfg_rtree.lua |   8 ++
 8 files changed, 233 insertions(+), 79 deletions(-)
 create mode 100644 test/box/lua/cfg_rtree.lua

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;
+	if (index_reserve(index, 0) != 0)
+		return -1;
 	return index_replace(index, NULL, tuple, DUP_INSERT, &unused);
 }
 
diff --git a/src/box/memtx_engine.h b/src/box/memtx_engine.h
index f562c66df..8b380bf3c 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 {
+	/**
+	 * 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..a5cae8f45 100644
--- 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;
+	});
+	struct memtx_engine *memtx = (struct memtx_engine *)base->engine;
+	return memtx_index_extent_reserve(memtx, RESERVE_EXTENTS_BEFORE_REPLACE);
+}
+
 static struct iterator *
 memtx_rtree_index_create_iterator(struct index *base,  enum iterator_type type,
 				  const char *key, uint32_t part_count)
@@ -333,7 +350,7 @@ static const struct index_vtab memtx_rtree_index_vtab = {
 	/* .compact = */ generic_index_compact,
 	/* .reset_stat = */ generic_index_reset_stat,
 	/* .begin_build = */ generic_index_begin_build,
-	/* .reserve = */ generic_index_reserve,
+	/* .reserve = */ memtx_rtree_index_reserve,
 	/* .build_next = */ generic_index_build_next,
 	/* .end_build = */ generic_index_end_build,
 };
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.
diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
index 672da2119..9ee9b5699 100644
--- a/src/lib/core/errinj.h
+++ b/src/lib/core/errinj.h
@@ -135,6 +135,7 @@ struct errinj {
 	_(ERRINJ_COIO_SENDFILE_CHUNK, ERRINJ_INT, {.iparam = -1}) \
 	_(ERRINJ_SWIM_FD_ONLY, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT, {.iparam = 0}) \
+	_(ERRINJ_INDEX_RESERVE, ERRINJ_BOOL, {.bparam = false})\
 
 ENUM0(errinj_id, ERRINJ_LIST);
 extern struct errinj errinjs[];
diff --git a/test/box/errinj.result b/test/box/errinj.result
index babe36b1b..c73fe3456 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -23,132 +23,134 @@ errinj.info()
 ---
 - ERRINJ_VY_RUN_WRITE_STMT_TIMEOUT:
     state: 0
-  ERRINJ_WAL_WRITE:
-    state: false
-  ERRINJ_RELAY_BREAK_LSN:
+  ERRINJ_WAL_BREAK_LSN:
     state: -1
-  ERRINJ_HTTPC_EXECUTE:
-    state: false
   ERRINJ_VYRUN_DATA_READ:
     state: false
-  ERRINJ_SWIM_FD_ONLY:
-    state: false
-  ERRINJ_SQL_NAME_NORMALIZATION:
-    state: false
   ERRINJ_VY_SCHED_TIMEOUT:
     state: 0
-  ERRINJ_COIO_SENDFILE_CHUNK:
-    state: -1
   ERRINJ_HTTP_RESPONSE_ADD_WAIT:
     state: false
-  ERRINJ_WAL_WRITE_PARTIAL:
-    state: -1
-  ERRINJ_VY_GC:
-    state: false
-  ERRINJ_WAL_DELAY:
-    state: false
-  ERRINJ_INDEX_ALLOC:
-    state: false
   ERRINJ_WAL_WRITE_EOF:
     state: false
-  ERRINJ_WAL_SYNC:
-    state: false
-  ERRINJ_BUILD_INDEX:
-    state: -1
   ERRINJ_BUILD_INDEX_DELAY:
     state: false
-  ERRINJ_VY_RUN_FILE_RENAME:
-    state: false
-  ERRINJ_VY_COMPACTION_DELAY:
-    state: false
-  ERRINJ_VY_DUMP_DELAY:
-    state: false
   ERRINJ_VY_DELAY_PK_LOOKUP:
     state: false
-  ERRINJ_VY_TASK_COMPLETE:
-    state: false
-  ERRINJ_PORT_DUMP:
+  ERRINJ_VY_POINT_ITER_WAIT:
     state: false
-  ERRINJ_WAL_BREAK_LSN:
-    state: -1
   ERRINJ_WAL_IO:
     state: false
-  ERRINJ_WAL_FALLOCATE:
-    state: 0
-  ERRINJ_DYN_MODULE_COUNT:
-    state: 0
   ERRINJ_VY_INDEX_FILE_RENAME:
     state: false
   ERRINJ_TUPLE_FORMAT_COUNT:
     state: -1
   ERRINJ_TUPLE_ALLOC:
     state: false
-  ERRINJ_VY_RUN_WRITE_DELAY:
+  ERRINJ_VY_RUN_FILE_RENAME:
     state: false
   ERRINJ_VY_READ_PAGE:
     state: false
   ERRINJ_RELAY_REPORT_INTERVAL:
     state: 0
-  ERRINJ_VY_LOG_FILE_RENAME:
-    state: false
-  ERRINJ_VY_READ_PAGE_TIMEOUT:
-    state: 0
+  ERRINJ_RELAY_BREAK_LSN:
+    state: -1
   ERRINJ_XLOG_META:
     state: false
-  ERRINJ_SIO_READ_MAX:
-    state: -1
   ERRINJ_SNAP_COMMIT_DELAY:
     state: false
-  ERRINJ_WAL_WRITE_DISK:
+  ERRINJ_VY_RUN_WRITE:
     state: false
-  ERRINJ_SNAP_WRITE_DELAY:
+  ERRINJ_BUILD_INDEX:
+    state: -1
+  ERRINJ_RELAY_FINAL_JOIN:
+    state: false
+  ERRINJ_REPLICA_JOIN_DELAY:
     state: false
   ERRINJ_LOG_ROTATE:
     state: false
-  ERRINJ_VY_RUN_WRITE:
+  ERRINJ_MEMTX_DELAY_GC:
     state: false
-  ERRINJ_CHECK_FORMAT_DELAY:
+  ERRINJ_XLOG_GARBAGE:
+    state: false
+  ERRINJ_VY_READ_PAGE_DELAY:
+    state: false
+  ERRINJ_INDEX_RESERVE:
+    state: false
+  ERRINJ_SWIM_FD_ONLY:
+    state: false
+  ERRINJ_WAL_WRITE:
     state: false
+  ERRINJ_HTTPC_EXECUTE:
+    state: false
+  ERRINJ_SQL_NAME_NORMALIZATION:
+    state: false
+  ERRINJ_WAL_WRITE_PARTIAL:
+    state: -1
+  ERRINJ_VY_GC:
+    state: false
+  ERRINJ_WAL_DELAY:
+    state: false
+  ERRINJ_XLOG_READ:
+    state: -1
+  ERRINJ_WAL_SYNC:
+    state: false
+  ERRINJ_VY_TASK_COMPLETE:
+    state: false
+  ERRINJ_PORT_DUMP:
+    state: false
+  ERRINJ_COIO_SENDFILE_CHUNK:
+    state: -1
+  ERRINJ_DYN_MODULE_COUNT:
+    state: 0
+  ERRINJ_SIO_READ_MAX:
+    state: -1
+  ERRINJ_RELAY_TIMEOUT:
+    state: 0
+  ERRINJ_VY_DUMP_DELAY:
+    state: false
+  ERRINJ_VY_SQUASH_TIMEOUT:
+    state: 0
   ERRINJ_VY_LOG_FLUSH_DELAY:
     state: false
-  ERRINJ_RELAY_FINAL_JOIN:
+  ERRINJ_RELAY_SEND_DELAY:
     state: false
-  ERRINJ_REPLICA_JOIN_DELAY:
+  ERRINJ_VY_COMPACTION_DELAY:
     state: false
-  ERRINJ_RELAY_FINAL_SLEEP:
+  ERRINJ_VY_LOG_FILE_RENAME:
     state: false
   ERRINJ_VY_RUN_DISCARD:
     state: false
   ERRINJ_WAL_ROTATE:
     state: false
-  ERRINJ_RELAY_EXIT_DELAY:
+  ERRINJ_VY_READ_PAGE_TIMEOUT:
     state: 0
-  ERRINJ_VY_POINT_ITER_WAIT:
+  ERRINJ_VY_INDEX_DUMP:
+    state: -1
+  ERRINJ_TUPLE_FIELD:
     state: false
-  ERRINJ_MEMTX_DELAY_GC:
+  ERRINJ_SNAP_WRITE_DELAY:
     state: false
   ERRINJ_IPROTO_TX_DELAY:
     state: false
-  ERRINJ_XLOG_READ:
-    state: -1
-  ERRINJ_TUPLE_FIELD:
+  ERRINJ_RELAY_EXIT_DELAY:
+    state: 0
+  ERRINJ_RELAY_FINAL_SLEEP:
     state: false
-  ERRINJ_XLOG_GARBAGE:
+  ERRINJ_WAL_WRITE_DISK:
     state: false
-  ERRINJ_VY_INDEX_DUMP:
-    state: -1
-  ERRINJ_VY_READ_PAGE_DELAY:
+  ERRINJ_CHECK_FORMAT_DELAY:
     state: false
   ERRINJ_TESTING:
     state: false
-  ERRINJ_RELAY_SEND_DELAY:
+  ERRINJ_VY_RUN_WRITE_DELAY:
     state: false
-  ERRINJ_VY_SQUASH_TIMEOUT:
+  ERRINJ_WAL_FALLOCATE:
     state: 0
   ERRINJ_VY_LOG_FLUSH:
     state: false
-  ERRINJ_RELAY_TIMEOUT:
-    state: 0
+  ERRINJ_INDEX_ALLOC:
+    state: false
 ...
 errinj.set("some-injection", true)
 ---
@@ -1781,3 +1783,100 @@ box.error.injection.get('ERRINJ_RELAY_TIMEOUT')
 ---
 - 0
 ...
+--
+-- gh-4619-RTREE-doesn't-handle-OOM-properly
+--
+test_run:cmd('create server rtree with script = "box/lua/cfg_rtree.lua"')
+---
+- true
+...
+test_run:cmd("start server rtree")
+---
+- true
+...
+test_run:cmd('switch rtree')
+---
+- true
+...
+math = require("math")
+---
+...
+rtreespace = box.schema.create_space('rtree', {if_not_exists = true})
+---
+...
+rtreespace:create_index('pk', {if_not_exists = true})
+---
+- unique: true
+  parts:
+  - type: unsigned
+    is_nullable: false
+    fieldno: 1
+  id: 0
+  space_id: 512
+  type: TREE
+  name: pk
+...
+rtreespace:create_index('target', {type='rtree', dimension = 3, parts={2, 'array'},unique = false, if_not_exists = true,})
+---
+- parts:
+  - type: array
+    is_nullable: false
+    fieldno: 2
+  dimension: 3
+  id: 1
+  type: RTREE
+  space_id: 512
+  name: target
+...
+count = 10
+---
+...
+for i = 1, count do box.space.rtree:insert{i, {(i + 1) -\
+    math.floor((i + 1)/7000) * 7000, (i + 2) - math.floor((i + 2)/7000) * 7000,\
+    (i + 3) - math.floor((i + 3)/7000) * 7000}} end
+---
+...
+rtreespace:count()
+---
+- 10
+...
+box.snapshot()
+---
+- ok
+...
+test_run:cmd('switch default')
+---
+- true
+...
+test_run:cmd("stop server rtree")
+---
+- true
+...
+test_run:cmd("start server rtree with crash_expected=True")
+---
+- false
+...
+fio = require('fio')
+---
+...
+fh = fio.open(fio.pathjoin(fio.cwd(), 'cfg_rtree.log'), {'O_RDONLY'})
+---
+...
+size = fh:seek(0, 'SEEK_END')
+---
+...
+fh:seek(-256, 'SEEK_END') ~= nil
+---
+- true
+...
+line = fh:read(256)
+---
+...
+fh:close()
+---
+- true
+...
+string.match(line, 'Failed to allocate') ~= nil
+---
+- true
+...
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
+--
+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")
+rtreespace = box.schema.create_space('rtree', {if_not_exists = true})
+rtreespace:create_index('pk', {if_not_exists = true})
+rtreespace:create_index('target', {type='rtree', dimension = 3, parts={2, 'array'},unique = false, if_not_exists = true,})
+count = 10
+for i = 1, count do box.space.rtree:insert{i, {(i + 1) -\
+    math.floor((i + 1)/7000) * 7000, (i + 2) - math.floor((i + 2)/7000) * 7000,\
+    (i + 3) - math.floor((i + 3)/7000) * 7000}} end
+rtreespace:count()
+box.snapshot()
+test_run:cmd('switch default')
+test_run:cmd("stop server rtree")
+test_run:cmd("start server rtree with crash_expected=True")
+fio = require('fio')
+fh = fio.open(fio.pathjoin(fio.cwd(), 'cfg_rtree.log'), {'O_RDONLY'})
+size = fh:seek(0, 'SEEK_END')
+fh:seek(-256, 'SEEK_END') ~= nil
+line = fh:read(256)
+fh:close()
+string.match(line, 'Failed to allocate') ~= nil
diff --git a/test/box/lua/cfg_rtree.lua b/test/box/lua/cfg_rtree.lua
new file mode 100644
index 000000000..f2d32ef7d
--- /dev/null
+++ b/test/box/lua/cfg_rtree.lua
@@ -0,0 +1,8 @@
+#!/usr/bin/env tarantool
+os = require('os')
+box.error.injection.set("ERRINJ_INDEX_RESERVE", true)
+box.cfg{
+    listen              = os.getenv("LISTEN"),
+}
+require('console').listen(os.getenv('ADMIN'))
+box.schema.user.grant('guest', 'read,write,execute', 'universe')
-- 
2.20.1 (Apple Git-117)

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

[Nikita Pettik]

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

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

[Olga Arkhangelskaia]

Hi Nikita! Thanks for the review!

I have changed everything according to your points.

10.02.2020 14:18, Nikita Pettik пишет:
> 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")

New diff:

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;
         return index_replace(index, NULL, tuple, DUP_INSERT, &unused);

diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
index a5cae8f45..612fcb2a9 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;
         });
         struct memtx_engine *memtx = (struct memtx_engine *)base->engine;

diff --git a/test/box/errinj.result b/test/box/errinj.result
index c73fe3456..c272d288d 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -1784,7 +1784,8 @@ box.error.injection.get('ERRINJ_RELAY_TIMEOUT')
  - 0
  ...
  --
--- gh-4619-RTREE-doesn't-handle-OOM-properly
+-- 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"')

diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
index 598e7da33..63c8b19fe 100644
--- a/test/box/errinj.test.lua
+++ b/test/box/errinj.test.lua
@@ -622,7 +622,8 @@ box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0)
  box.error.injection.get('ERRINJ_RELAY_TIMEOUT')

  --
--- gh-4619-RTREE-doesn't-handle-OOM-properly
+-- 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")

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

[Olga Arkhangelskaia]

Hi Nikita! Thanks for the review.

I have fixed everything you have pointed out. Diff can be found below.

10.02.2020 14:18, Nikita Pettik пишет:
> 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")
  diff --git a/src/box/index.cc b/src/box/index.cc
-index 4e4867118..c2fc00867 100644
+index 4e4867118..750838bef 100644
  --- a/src/box/index.cc
  +++ b/src/box/index.cc
-@@ -733,6 +733,13 @@ int
+@@ -733,6 +733,8 @@ 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;

diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
-index 8badad797..612fcb2a9 100644
+index 8badad797..a5cae8f45 100644
  --- a/src/box/memtx_rtree.c
  +++ b/src/box/memtx_rtree.c
-@@ -242,6 +242,24 @@ memtx_rtree_index_replace(struct index *base, 
struct tuple *old_tuple,
+@@ -242,6 +242,23 @@ memtx_rtree_index_replace(struct index *base, 
struct tuple *old_tuple,
         return 0;
}

@@ -82,14 +109,13 @@
  +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 during any operation
-+         * on rtree, because there is no error handling in the
-+         * rtree lib.
-+ */
++       * 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, "mempool", "new 
slab");
++              diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "slab 
allocator", "memtx extent");
  +              return -1;
  + });
  +      struct memtx_engine *memtx = (struct memtx_engine *)base->engine;

  diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
-index 03c088677..63c8b19fe 100644
+index 03c088677..598e7da33 100644
  --- a/test/box/errinj.test.lua
  +++ b/test/box/errinj.test.lua
-@@ -620,3 +620,31 @@ box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0.5)
+@@ -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: make sure that if OOM takes place during rtree recovery,
-+-- Tarantool instance will fail gracefully.
++-- gh-4619-RTREE-doesn't-handle-OOM-properly
  +--
  +test_run:cmd('create server rtree with script = 
"box/lua/cfg_rtree.lua"')
  +test_run:cmd("start server rtree")

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

[Nikita Pettik]

On 11 Feb 13:51, Olga Arkhangelskaia wrote:
> Hi Nikita! Thanks for the review.
> 
> I have fixed everything you have pointed out. Diff can be found below.

LGTM
 
> 10.02.2020 14:18, Nikita Pettik пишет:
> > On 10 Feb 10:47, Olga Arkhangelskaia wrote:
>  diff --git a/src/box/index.cc b/src/box/index.cc
> -index 4e4867118..c2fc00867 100644
> +index 4e4867118..750838bef 100644
>  --- a/src/box/index.cc
>  +++ b/src/box/index.cc
> -@@ -733,6 +733,13 @@ int
> +@@ -733,6 +733,8 @@ 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;
> 
> diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
> -index 8badad797..612fcb2a9 100644
> +index 8badad797..a5cae8f45 100644
>  --- a/src/box/memtx_rtree.c
>  +++ b/src/box/memtx_rtree.c
> -@@ -242,6 +242,24 @@ memtx_rtree_index_replace(struct index *base, struct
> tuple *old_tuple,
> +@@ -242,6 +242,23 @@ memtx_rtree_index_replace(struct index *base, struct
> tuple *old_tuple,
>         return 0;
> }
> 
> @@ -82,14 +109,13 @@
>  +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 during any operation
> -+         * on rtree, because there is no error handling in the
> -+         * rtree lib.
> -+ */
> ++       * 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, "mempool", "new
> slab");
> ++              diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "slab allocator",
> "memtx extent");
>  +              return -1;
>  + });
>  +      struct memtx_engine *memtx = (struct memtx_engine *)base->engine;
> 
>  diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
> -index 03c088677..63c8b19fe 100644
> +index 03c088677..598e7da33 100644
>  --- a/test/box/errinj.test.lua
>  +++ b/test/box/errinj.test.lua
> -@@ -620,3 +620,31 @@ box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0.5)
> +@@ -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: make sure that if OOM takes place during rtree recovery,
> -+-- Tarantool instance will fail gracefully.
> ++-- gh-4619-RTREE-doesn't-handle-OOM-properly
>  +--
>  +test_run:cmd('create server rtree with script = "box/lua/cfg_rtree.lua"')
>  +test_run:cmd("start server rtree")

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

[Sergey Ostanevich]

Hi!

Thanks for the patch! 

I wonder why do you use additional by-ref argument to pass an integer
result instead of redefining the interface into:

    int
    rtree_insert(struct rtree *tree, struct rtree_rect *rect, record_t obj)

instead, so that all uses of a form:

    int err = 0;
    ...
    rtree_insert(&index->tree, &rect, new_tuple, &err);
    if (err == 1) {
        diag_set(OutOfMemory, sizeof(struct rtree),
                "rtree", "rtree_page");
        return -1;
    }

into a much simpler:

    if (rtree_insert(&index->tree, &rect, new_tuple, &err)) {
        diag_set(OutOfMemory, sizeof(struct rtree),
                "rtree", "rtree_page");
        return -1; 
    }   

Sergos

On 11 Feb 15:42, Nikita Pettik wrote:
> On 11 Feb 13:51, Olga Arkhangelskaia wrote:
> > Hi Nikita! Thanks for the review.
> > 
> > I have fixed everything you have pointed out. Diff can be found below.
> 
> LGTM
>  
> > 10.02.2020 14:18, Nikita Pettik пишет:
> > > On 10 Feb 10:47, Olga Arkhangelskaia wrote:
> >  diff --git a/src/box/index.cc b/src/box/index.cc
> > -index 4e4867118..c2fc00867 100644
> > +index 4e4867118..750838bef 100644
> >  --- a/src/box/index.cc
> >  +++ b/src/box/index.cc
> > -@@ -733,6 +733,13 @@ int
> > +@@ -733,6 +733,8 @@ 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;
> > 
> > diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
> > -index 8badad797..612fcb2a9 100644
> > +index 8badad797..a5cae8f45 100644
> >  --- a/src/box/memtx_rtree.c
> >  +++ b/src/box/memtx_rtree.c
> > -@@ -242,6 +242,24 @@ memtx_rtree_index_replace(struct index *base, struct
> > tuple *old_tuple,
> > +@@ -242,6 +242,23 @@ memtx_rtree_index_replace(struct index *base, struct
> > tuple *old_tuple,
> >         return 0;
> > }
> > 
> > @@ -82,14 +109,13 @@
> >  +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 during any operation
> > -+         * on rtree, because there is no error handling in the
> > -+         * rtree lib.
> > -+ */
> > ++       * 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, "mempool", "new
> > slab");
> > ++              diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "slab allocator",
> > "memtx extent");
> >  +              return -1;
> >  + });
> >  +      struct memtx_engine *memtx = (struct memtx_engine *)base->engine;
> > 
> >  diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
> > -index 03c088677..63c8b19fe 100644
> > +index 03c088677..598e7da33 100644
> >  --- a/test/box/errinj.test.lua
> >  +++ b/test/box/errinj.test.lua
> > -@@ -620,3 +620,31 @@ box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0.5)
> > +@@ -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: make sure that if OOM takes place during rtree recovery,
> > -+-- Tarantool instance will fail gracefully.
> > ++-- gh-4619-RTREE-doesn't-handle-OOM-properly
> >  +--
> >  +test_run:cmd('create server rtree with script = "box/lua/cfg_rtree.lua"')
> >  +test_run:cmd("start server rtree")

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

[Olga Arkhangelskaia]

Hi! Thanks for the review! But I am a bit confused.

18.02.2020 15:46, Sergey Ostanevich пишет:
> Hi!
>
> Thanks for the patch!
>
> I wonder why do you use additional by-ref argument to pass an integer
> result instead of redefining the interface into:
>
>      int
>      rtree_insert(struct rtree *tree, struct rtree_rect *rect, record_t obj)
>
> instead, so that all uses of a form:
>
>      int err = 0;
>      ...
>      rtree_insert(&index->tree, &rect, new_tuple, &err);


There is no way rtree returns an error. Library itself can not fail.  
And the best way is to reserve pages before malloc itself in the library.

We have discussed in the very beginning with Konstantin Osipov and later 
with Aleksandr Lyapunov.

https://lists.tarantool.org/pipermail/tarantool-patches/2019-November/012392.html

So we reserve memory instead of returning an error.

>      if (err == 1) {
>          diag_set(OutOfMemory, sizeof(struct rtree),
>                  "rtree", "rtree_page");
>          return -1;
>      }
>
> into a much simpler:
>
>      if (rtree_insert(&index->tree, &rect, new_tuple, &err)) {
>          diag_set(OutOfMemory, sizeof(struct rtree),
>                  "rtree", "rtree_page");
>          return -1;
>      }
>
> Sergos
>
> On 11 Feb 15:42, Nikita Pettik wrote:
>> On 11 Feb 13:51, Olga Arkhangelskaia wrote:
>>> Hi Nikita! Thanks for the review.
>>>
>>> I have fixed everything you have pointed out. Diff can be found below.
>> LGTM
>>   
>>> 10.02.2020 14:18, Nikita Pettik пишет:
>>>> On 10 Feb 10:47, Olga Arkhangelskaia wrote:
>>>   diff --git a/src/box/index.cc b/src/box/index.cc
>>> -index 4e4867118..c2fc00867 100644
>>> +index 4e4867118..750838bef 100644
>>>   --- a/src/box/index.cc
>>>   +++ b/src/box/index.cc
>>> -@@ -733,6 +733,13 @@ int
>>> +@@ -733,6 +733,8 @@ 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;
>>>
>>> diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
>>> -index 8badad797..612fcb2a9 100644
>>> +index 8badad797..a5cae8f45 100644
>>>   --- a/src/box/memtx_rtree.c
>>>   +++ b/src/box/memtx_rtree.c
>>> -@@ -242,6 +242,24 @@ memtx_rtree_index_replace(struct index *base, struct
>>> tuple *old_tuple,
>>> +@@ -242,6 +242,23 @@ memtx_rtree_index_replace(struct index *base, struct
>>> tuple *old_tuple,
>>>          return 0;
>>> }
>>>
>>> @@ -82,14 +109,13 @@
>>>   +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 during any operation
>>> -+         * on rtree, because there is no error handling in the
>>> -+         * rtree lib.
>>> -+ */
>>> ++       * 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, "mempool", "new
>>> slab");
>>> ++              diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "slab allocator",
>>> "memtx extent");
>>>   +              return -1;
>>>   + });
>>>   +      struct memtx_engine *memtx = (struct memtx_engine *)base->engine;
>>>
>>>   diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
>>> -index 03c088677..63c8b19fe 100644
>>> +index 03c088677..598e7da33 100644
>>>   --- a/test/box/errinj.test.lua
>>>   +++ b/test/box/errinj.test.lua
>>> -@@ -620,3 +620,31 @@ box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0.5)
>>> +@@ -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: make sure that if OOM takes place during rtree recovery,
>>> -+-- Tarantool instance will fail gracefully.
>>> ++-- gh-4619-RTREE-doesn't-handle-OOM-properly
>>>   +--
>>>   +test_run:cmd('create server rtree with script = "box/lua/cfg_rtree.lua"')
>>>   +test_run:cmd("start server rtree")

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

[Sergey Ostanevich]

Hi!

I am sorry, I took the OKriw/gh-4619-RTREE-doesnt-handle-OOM-properly
branch for review, instead of OKriw/gh-RTREE-doesnt-handle-OOM-properly

I can see memtx_index_extent_reserve() has an error injection already.
Is it not sufficient, so you still need a new one in
memtx_rtree_index_reserve() ?

Otherwise looks good.

Regards,
Sergos


On 18 фев 21:20, Olga Arkhangelskaia wrote:
> Hi! Thanks for the review! But I am a bit confused.
> 
> 18.02.2020 15:46, Sergey Ostanevich пишет:
> > Hi!
> > 
> > Thanks for the patch!
> > 
> > I wonder why do you use additional by-ref argument to pass an integer
> > result instead of redefining the interface into:
> > 
> >      int
> >      rtree_insert(struct rtree *tree, struct rtree_rect *rect, record_t obj)
> > 
> > instead, so that all uses of a form:
> > 
> >      int err = 0;
> >      ...
> >      rtree_insert(&index->tree, &rect, new_tuple, &err);
> 
> 
> There is no way rtree returns an error. Library itself can not fail.  And
> the best way is to reserve pages before malloc itself in the library.
> 
> We have discussed in the very beginning with Konstantin Osipov and later
> with Aleksandr Lyapunov.
> 
> https://lists.tarantool.org/pipermail/tarantool-patches/2019-November/012392.html
> 
> So we reserve memory instead of returning an error.
> 
> >      if (err == 1) {
> >          diag_set(OutOfMemory, sizeof(struct rtree),
> >                  "rtree", "rtree_page");
> >          return -1;
> >      }
> > 
> > into a much simpler:
> > 
> >      if (rtree_insert(&index->tree, &rect, new_tuple, &err)) {
> >          diag_set(OutOfMemory, sizeof(struct rtree),
> >                  "rtree", "rtree_page");
> >          return -1;
> >      }
> > 
> > Sergos
> > 
> > On 11 Feb 15:42, Nikita Pettik wrote:
> > > On 11 Feb 13:51, Olga Arkhangelskaia wrote:
> > > > Hi Nikita! Thanks for the review.
> > > > 
> > > > I have fixed everything you have pointed out. Diff can be found below.
> > > LGTM
> > > > 10.02.2020 14:18, Nikita Pettik пишет:
> > > > > On 10 Feb 10:47, Olga Arkhangelskaia wrote:
> > > >   diff --git a/src/box/index.cc b/src/box/index.cc
> > > > -index 4e4867118..c2fc00867 100644
> > > > +index 4e4867118..750838bef 100644
> > > >   --- a/src/box/index.cc
> > > >   +++ b/src/box/index.cc
> > > > -@@ -733,6 +733,13 @@ int
> > > > +@@ -733,6 +733,8 @@ 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;
> > > > 
> > > > diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
> > > > -index 8badad797..612fcb2a9 100644
> > > > +index 8badad797..a5cae8f45 100644
> > > >   --- a/src/box/memtx_rtree.c
> > > >   +++ b/src/box/memtx_rtree.c
> > > > -@@ -242,6 +242,24 @@ memtx_rtree_index_replace(struct index *base, struct
> > > > tuple *old_tuple,
> > > > +@@ -242,6 +242,23 @@ memtx_rtree_index_replace(struct index *base, struct
> > > > tuple *old_tuple,
> > > >          return 0;
> > > > }
> > > > 
> > > > @@ -82,14 +109,13 @@
> > > >   +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 during any operation
> > > > -+         * on rtree, because there is no error handling in the
> > > > -+         * rtree lib.
> > > > -+ */
> > > > ++       * 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, "mempool", "new
> > > > slab");
> > > > ++              diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "slab allocator",
> > > > "memtx extent");
> > > >   +              return -1;
> > > >   + });
> > > >   +      struct memtx_engine *memtx = (struct memtx_engine *)base->engine;
> > > > 
> > > >   diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
> > > > -index 03c088677..63c8b19fe 100644
> > > > +index 03c088677..598e7da33 100644
> > > >   --- a/test/box/errinj.test.lua
> > > >   +++ b/test/box/errinj.test.lua
> > > > -@@ -620,3 +620,31 @@ box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0.5)
> > > > +@@ -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: make sure that if OOM takes place during rtree recovery,
> > > > -+-- Tarantool instance will fail gracefully.
> > > > ++-- gh-4619-RTREE-doesn't-handle-OOM-properly
> > > >   +--
> > > >   +test_run:cmd('create server rtree with script = "box/lua/cfg_rtree.lua"')
> > > >   +test_run:cmd("start server rtree")

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

[Olga Arkhangelskaia]

Hi Sergey! I need time to check everything!

I can not use error injection that exists in memtx_index_extent_reserve(),

because it is used  in memtx_space_replace_all_keys ant it is called 
prior memtx_rtree_index_reserve() .

If we use memtx_index_extent_reserve(), injection we will not reach the 
place where error with rtree occures.

So to make sure that we fixed the problem we need 
memtx_rtree_index_reserve() to fail.


19.02.2020 19:28, Sergey Ostanevich пишет:
> Hi!
>
> I am sorry, I took the OKriw/gh-4619-RTREE-doesnt-handle-OOM-properly
> branch for review, instead of OKriw/gh-RTREE-doesnt-handle-OOM-properly
>
> I can see memtx_index_extent_reserve() has an error injection already.
> Is it not sufficient, so you still need a new one in
> memtx_rtree_index_reserve() ?

> Otherwise looks good.
>
> Regards,
> Sergos
>
>
> On 18 фев 21:20, Olga Arkhangelskaia wrote:
>> Hi! Thanks for the review! But I am a bit confused.
>>
>> 18.02.2020 15:46, Sergey Ostanevich пишет:
>>> Hi!
>>>
>>> Thanks for the patch!
>>>
>>> I wonder why do you use additional by-ref argument to pass an integer
>>> result instead of redefining the interface into:
>>>
>>>       int
>>>       rtree_insert(struct rtree *tree, struct rtree_rect *rect, record_t obj)
>>>
>>> instead, so that all uses of a form:
>>>
>>>       int err = 0;
>>>       ...
>>>       rtree_insert(&index->tree, &rect, new_tuple, &err);
>>
>> There is no way rtree returns an error. Library itself can not fail.  And
>> the best way is to reserve pages before malloc itself in the library.
>>
>> We have discussed in the very beginning with Konstantin Osipov and later
>> with Aleksandr Lyapunov.
>>
>> https://lists.tarantool.org/pipermail/tarantool-patches/2019-November/012392.html
>>
>> So we reserve memory instead of returning an error.
>>
>>>       if (err == 1) {
>>>           diag_set(OutOfMemory, sizeof(struct rtree),
>>>                   "rtree", "rtree_page");
>>>           return -1;
>>>       }
>>>
>>> into a much simpler:
>>>
>>>       if (rtree_insert(&index->tree, &rect, new_tuple, &err)) {
>>>           diag_set(OutOfMemory, sizeof(struct rtree),
>>>                   "rtree", "rtree_page");
>>>           return -1;
>>>       }
>>>
>>> Sergos
>>>
>>> On 11 Feb 15:42, Nikita Pettik wrote:
>>>> On 11 Feb 13:51, Olga Arkhangelskaia wrote:
>>>>> Hi Nikita! Thanks for the review.
>>>>>
>>>>> I have fixed everything you have pointed out. Diff can be found below.
>>>> LGTM
>>>>> 10.02.2020 14:18, Nikita Pettik пишет:
>>>>>> On 10 Feb 10:47, Olga Arkhangelskaia wrote:
>>>>>    diff --git a/src/box/index.cc b/src/box/index.cc
>>>>> -index 4e4867118..c2fc00867 100644
>>>>> +index 4e4867118..750838bef 100644
>>>>>    --- a/src/box/index.cc
>>>>>    +++ b/src/box/index.cc
>>>>> -@@ -733,6 +733,13 @@ int
>>>>> +@@ -733,6 +733,8 @@ 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;
>>>>>
>>>>> diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
>>>>> -index 8badad797..612fcb2a9 100644
>>>>> +index 8badad797..a5cae8f45 100644
>>>>>    --- a/src/box/memtx_rtree.c
>>>>>    +++ b/src/box/memtx_rtree.c
>>>>> -@@ -242,6 +242,24 @@ memtx_rtree_index_replace(struct index *base, struct
>>>>> tuple *old_tuple,
>>>>> +@@ -242,6 +242,23 @@ memtx_rtree_index_replace(struct index *base, struct
>>>>> tuple *old_tuple,
>>>>>           return 0;
>>>>> }
>>>>>
>>>>> @@ -82,14 +109,13 @@
>>>>>    +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 during any operation
>>>>> -+         * on rtree, because there is no error handling in the
>>>>> -+         * rtree lib.
>>>>> -+ */
>>>>> ++       * 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, "mempool", "new
>>>>> slab");
>>>>> ++              diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "slab allocator",
>>>>> "memtx extent");
>>>>>    +              return -1;
>>>>>    + });
>>>>>    +      struct memtx_engine *memtx = (struct memtx_engine *)base->engine;
>>>>>
>>>>>    diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
>>>>> -index 03c088677..63c8b19fe 100644
>>>>> +index 03c088677..598e7da33 100644
>>>>>    --- a/test/box/errinj.test.lua
>>>>>    +++ b/test/box/errinj.test.lua
>>>>> -@@ -620,3 +620,31 @@ box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0.5)
>>>>> +@@ -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: make sure that if OOM takes place during rtree recovery,
>>>>> -+-- Tarantool instance will fail gracefully.
>>>>> ++-- gh-4619-RTREE-doesn't-handle-OOM-properly
>>>>>    +--
>>>>>    +test_run:cmd('create server rtree with script = "box/lua/cfg_rtree.lua"')
>>>>>    +test_run:cmd("start server rtree")

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

[Sergey Ostanevich]

Olga,

Thanks for explanation! LGTM.

Sergos

On 03 мар 14:50, Olga Arkhangelskaia wrote:
> Hi Sergey! I need time to check everything!
> 
> I can not use error injection that exists in memtx_index_extent_reserve(),
> 
> because it is used  in memtx_space_replace_all_keys ant it is called prior
> memtx_rtree_index_reserve() .
> 
> If we use memtx_index_extent_reserve(), injection we will not reach the
> place where error with rtree occures.
> 
> So to make sure that we fixed the problem we need
> memtx_rtree_index_reserve() to fail.
> 
> 
> 19.02.2020 19:28, Sergey Ostanevich пишет:
> > Hi!
> > 
> > I am sorry, I took the OKriw/gh-4619-RTREE-doesnt-handle-OOM-properly
> > branch for review, instead of OKriw/gh-RTREE-doesnt-handle-OOM-properly
> > 
> > I can see memtx_index_extent_reserve() has an error injection already.
> > Is it not sufficient, so you still need a new one in
> > memtx_rtree_index_reserve() ?
> 
> > Otherwise looks good.
> > 
> > Regards,
> > Sergos
> > 
> > 
> > On 18 фев 21:20, Olga Arkhangelskaia wrote:
> > > Hi! Thanks for the review! But I am a bit confused.
> > > 
> > > 18.02.2020 15:46, Sergey Ostanevich пишет:
> > > > Hi!
> > > > 
> > > > Thanks for the patch!
> > > > 
> > > > I wonder why do you use additional by-ref argument to pass an integer
> > > > result instead of redefining the interface into:
> > > > 
> > > >       int
> > > >       rtree_insert(struct rtree *tree, struct rtree_rect *rect, record_t obj)
> > > > 
> > > > instead, so that all uses of a form:
> > > > 
> > > >       int err = 0;
> > > >       ...
> > > >       rtree_insert(&index->tree, &rect, new_tuple, &err);
> > > 
> > > There is no way rtree returns an error. Library itself can not fail.  And
> > > the best way is to reserve pages before malloc itself in the library.
> > > 
> > > We have discussed in the very beginning with Konstantin Osipov and later
> > > with Aleksandr Lyapunov.
> > > 
> > > https://lists.tarantool.org/pipermail/tarantool-patches/2019-November/012392.html
> > > 
> > > So we reserve memory instead of returning an error.
> > > 
> > > >       if (err == 1) {
> > > >           diag_set(OutOfMemory, sizeof(struct rtree),
> > > >                   "rtree", "rtree_page");
> > > >           return -1;
> > > >       }
> > > > 
> > > > into a much simpler:
> > > > 
> > > >       if (rtree_insert(&index->tree, &rect, new_tuple, &err)) {
> > > >           diag_set(OutOfMemory, sizeof(struct rtree),
> > > >                   "rtree", "rtree_page");
> > > >           return -1;
> > > >       }
> > > > 
> > > > Sergos
> > > > 
> > > > On 11 Feb 15:42, Nikita Pettik wrote:
> > > > > On 11 Feb 13:51, Olga Arkhangelskaia wrote:
> > > > > > Hi Nikita! Thanks for the review.
> > > > > > 
> > > > > > I have fixed everything you have pointed out. Diff can be found below.
> > > > > LGTM
> > > > > > 10.02.2020 14:18, Nikita Pettik пишет:
> > > > > > > On 10 Feb 10:47, Olga Arkhangelskaia wrote:
> > > > > >    diff --git a/src/box/index.cc b/src/box/index.cc
> > > > > > -index 4e4867118..c2fc00867 100644
> > > > > > +index 4e4867118..750838bef 100644
> > > > > >    --- a/src/box/index.cc
> > > > > >    +++ b/src/box/index.cc
> > > > > > -@@ -733,6 +733,13 @@ int
> > > > > > +@@ -733,6 +733,8 @@ 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;
> > > > > > 
> > > > > > diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
> > > > > > -index 8badad797..612fcb2a9 100644
> > > > > > +index 8badad797..a5cae8f45 100644
> > > > > >    --- a/src/box/memtx_rtree.c
> > > > > >    +++ b/src/box/memtx_rtree.c
> > > > > > -@@ -242,6 +242,24 @@ memtx_rtree_index_replace(struct index *base, struct
> > > > > > tuple *old_tuple,
> > > > > > +@@ -242,6 +242,23 @@ memtx_rtree_index_replace(struct index *base, struct
> > > > > > tuple *old_tuple,
> > > > > >           return 0;
> > > > > > }
> > > > > > 
> > > > > > @@ -82,14 +109,13 @@
> > > > > >    +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 during any operation
> > > > > > -+         * on rtree, because there is no error handling in the
> > > > > > -+         * rtree lib.
> > > > > > -+ */
> > > > > > ++       * 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, "mempool", "new
> > > > > > slab");
> > > > > > ++              diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "slab allocator",
> > > > > > "memtx extent");
> > > > > >    +              return -1;
> > > > > >    + });
> > > > > >    +      struct memtx_engine *memtx = (struct memtx_engine *)base->engine;
> > > > > > 
> > > > > >    diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
> > > > > > -index 03c088677..63c8b19fe 100644
> > > > > > +index 03c088677..598e7da33 100644
> > > > > >    --- a/test/box/errinj.test.lua
> > > > > >    +++ b/test/box/errinj.test.lua
> > > > > > -@@ -620,3 +620,31 @@ box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0.5)
> > > > > > +@@ -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: make sure that if OOM takes place during rtree recovery,
> > > > > > -+-- Tarantool instance will fail gracefully.
> > > > > > ++-- gh-4619-RTREE-doesn't-handle-OOM-properly
> > > > > >    +--
> > > > > >    +test_run:cmd('create server rtree with script = "box/lua/cfg_rtree.lua"')
> > > > > >    +test_run:cmd("start server rtree")

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

[Nikita Pettik]

On 03 Mar 14:50, Olga Arkhangelskaia wrote:
> Hi Sergey! I need time to check everything!
> 
> I can not use error injection that exists in memtx_index_extent_reserve(),
> 
> because it is used  in memtx_space_replace_all_keys ant it is called prior
> memtx_rtree_index_reserve() .
> 
> If we use memtx_index_extent_reserve(), injection we will not reach the
> place where error with rtree occures.
> 
> So to make sure that we fixed the problem we need
> memtx_rtree_index_reserve() to fail.
> 

Since now patch received two acks, it is ready to be pushed.
Olga, could you please rebase branch on the top of master (there's several
merge conflicts) and provide brief @changelog (a few sentences for casual
Tarantool users concerning visible changes this patch introduces)? As soon
as it is ready, I'll push it. Thanks.

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

[Olga Arkhangelskaia]

Done.

05.03.2020 0:18, Nikita Pettik пишет:
> On 03 Mar 14:50, Olga Arkhangelskaia wrote:
>> Hi Sergey! I need time to check everything!
>>
>> I can not use error injection that exists in memtx_index_extent_reserve(),
>>
>> because it is used  in memtx_space_replace_all_keys ant it is called prior
>> memtx_rtree_index_reserve() .
>>
>> If we use memtx_index_extent_reserve(), injection we will not reach the
>> place where error with rtree occures.
>>
>> So to make sure that we fixed the problem we need
>> memtx_rtree_index_reserve() to fail.
>>
> Since now patch received two acks, it is ready to be pushed.
> Olga, could you please rebase branch on the top of master (there's several
> merge conflicts) and provide brief @changelog (a few sentences for casual
> Tarantool users concerning visible changes this patch introduces)? As soon
> as it is ready, I'll push it. Thanks.
>

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

[Nikita Pettik]

On 10 Mar 14:25, Olga Arkhangelskaia wrote:
> Done.

Pushed to master, 2.3, 2.2 and 1.10. Updated changelogs for
branches correspondingly.
 
> 05.03.2020 0:18, Nikita Pettik пишет:
> > On 03 Mar 14:50, Olga Arkhangelskaia wrote:
> > > Hi Sergey! I need time to check everything!
> > > 
> > > I can not use error injection that exists in memtx_index_extent_reserve(),
> > > 
> > > because it is used  in memtx_space_replace_all_keys ant it is called prior
> > > memtx_rtree_index_reserve() .
> > > 
> > > If we use memtx_index_extent_reserve(), injection we will not reach the
> > > place where error with rtree occures.
> > > 
> > > So to make sure that we fixed the problem we need
> > > memtx_rtree_index_reserve() to fail.
> > > 
> > Since now patch received two acks, it is ready to be pushed.
> > Olga, could you please rebase branch on the top of master (there's several
> > merge conflicts) and provide brief @changelog (a few sentences for casual
> > Tarantool users concerning visible changes this patch introduces)? As soon
> > as it is ready, I'll push it. Thanks.
> >