[Tarantool-patches] [PATCH v4 0/4] Safe truncation and deletion

[Tarantool-patches] [PATCH v4 0/4] Safe truncation and deletion

[Ilya Kosarev]

space:truncate() and space:delete() could fail on memory allocations
when reaching memtx_memory limit. As far as it is quite an ill
behaviour, it is fixed in this patchset through memtx quota strictness
adjustment. Now it can be overused if needed.
Also possible bps_tree_create_leaf NULL dereference issue is fixed.

Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-3807-safe-alloc-on-truncation
Issue: https://github.com/tarantool/tarantool/issues/3807

Changes in v2:
  Approach changed completely: now we are not trying to allocate
  service tuples in some safe way, but increasing memtx quota so
  that space:truncate() and space:delete() won't fail on allocation.

Changes in v3:
  Now we are not increasing memtx quota. Instead we just set a flag to
  allow quota overuse for space:truncate() and space:delete().

Changes in v4:
  improved quota on/off flag style
  took into account fail cases for box_upsert in space_truncate
  now we are switching the quota off for deletion only in case there is
  OutOfMemory error in diag
  removed extra checks
  added tests

Ilya Kosarev (4):
  small: bump small version
  b-tree: return NULL on matras_alloc fail
  memtx: allow quota overuse for truncation and deletion
  test: add tests for truncation and deletion

 src/box/box.cc                       |  27 ++++++-
 src/box/memtx_space.c                |  33 ++++++--
 src/lib/core/errinj.h                |   1 +
 src/lib/salad/bps_tree.h             |  22 +++++-
 src/lib/small                        |   2 +-
 test/box/errinj.result               |   1 +
 test/engine/engine.cfg               |   6 ++
 test/engine/low_memory.lua           |   8 ++
 test/engine/stress_delete.result     | 111 +++++++++++++++++++++++++++
 test/engine/stress_delete.test.lua   |  55 +++++++++++++
 test/engine/stress_truncate.result   | 103 +++++++++++++++++++++++++
 test/engine/stress_truncate.test.lua |  52 +++++++++++++
 12 files changed, 408 insertions(+), 13 deletions(-)
 create mode 100644 test/engine/low_memory.lua
 create mode 100644 test/engine/stress_delete.result
 create mode 100644 test/engine/stress_delete.test.lua
 create mode 100644 test/engine/stress_truncate.result
 create mode 100644 test/engine/stress_truncate.test.lua

-- 
2.17.1

[Tarantool-patches] [PATCH v4 1/4] small: bump small version

[Ilya Kosarev]

---
 src/lib/small | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/lib/small b/src/lib/small
index 3df505017..8fe6374fe 160000
--- a/src/lib/small
+++ b/src/lib/small
@@ -1 +1 @@
-Subproject commit 3df5050171d040bfe5717b4bddf57da3b312ffe4
+Subproject commit 8fe6374fe52a437daa68be97b1a0cbd6a1345eca
-- 
2.17.1

[Tarantool-patches] [PATCH v4 2/4] b-tree: return NULL on matras_alloc fail

[Ilya Kosarev]

In bps_tree_create_leaf we use matras_alloc in case
bps_tree_garbage_pop didn't work out. However it also might not
succeed. Then we need to return NULL instead of dereferencing NULL
pointer.

Part of #3807
---
 src/lib/salad/bps_tree.h | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/src/lib/salad/bps_tree.h b/src/lib/salad/bps_tree.h
index d28b53f53..d71340143 100644
--- a/src/lib/salad/bps_tree.h
+++ b/src/lib/salad/bps_tree.h
@@ -2147,8 +2147,11 @@ bps_tree_create_leaf(struct bps_tree *tree, bps_tree_block_id_t *id)
 {
 	struct bps_leaf *res = (struct bps_leaf *)
 			       bps_tree_garbage_pop(tree, id);
-	if (!res)
-		res = (struct bps_leaf *)matras_alloc(&tree->matras, id);
+	if (!res) {
+		res = (struct bps_leaf *) matras_alloc(&tree->matras, id);
+		if (!res)
+			return NULL;
+	}
 	res->header.type = BPS_TREE_BT_LEAF;
 	tree->leaf_count++;
 	return res;
@@ -2162,8 +2165,11 @@ bps_tree_create_inner(struct bps_tree *tree, bps_tree_block_id_t *id)
 {
 	struct bps_inner *res = (struct bps_inner *)
 				bps_tree_garbage_pop(tree, id);
-	if (!res)
-		res = (struct bps_inner *)matras_alloc(&tree->matras, id);
+	if (!res) {
+		res = (struct bps_inner *) matras_alloc(&tree->matras, id);
+		if (!res)
+			return NULL;
+	}
 	res->header.type = BPS_TREE_BT_INNER;
 	tree->inner_count++;
 	return res;
@@ -3472,6 +3478,8 @@ bps_tree_process_insert_leaf(struct bps_tree *tree,
 	}
 	bps_tree_block_id_t new_block_id = (bps_tree_block_id_t)(-1);
 	struct bps_leaf *new_leaf = bps_tree_create_leaf(tree, &new_block_id);
+	if (!new_leaf)
+		return -1;
 
 	leaf_path_elem->block = (struct bps_leaf *)
 	bps_tree_touch_block(tree, leaf_path_elem->block_id);
@@ -3710,6 +3718,8 @@ bps_tree_process_insert_leaf(struct bps_tree *tree,
 		bps_tree_block_id_t new_root_id = (bps_tree_block_id_t)(-1);
 		struct bps_inner *new_root = bps_tree_create_inner(tree,
 				&new_root_id);
+		if (!new_root)
+			return -1;
 		new_root->header.size = 2;
 		new_root->child_ids[0] = tree->root_id;
 		new_root->child_ids[1] = new_block_id;
@@ -3835,6 +3845,8 @@ bps_tree_process_insert_inner(struct bps_tree *tree,
 	bps_tree_block_id_t new_block_id = (bps_tree_block_id_t)(-1);
 	struct bps_inner *new_inner = bps_tree_create_inner(tree,
 			&new_block_id);
+	if (!new_inner)
+		return -1;
 
 	new_inner->header.size = 0;
 	struct bps_inner_path_elem new_path_elem;
@@ -4030,6 +4042,8 @@ bps_tree_process_insert_inner(struct bps_tree *tree,
 		bps_tree_block_id_t new_root_id = (bps_tree_block_id_t)(-1);
 		struct bps_inner *new_root =
 			bps_tree_create_inner(tree, &new_root_id);
+		if (!new_root)
+			return -1;
 		new_root->header.size = 2;
 		new_root->child_ids[0] = tree->root_id;
 		new_root->child_ids[1] = new_block_id;
-- 
2.17.1

[Tarantool-patches] [PATCH v4 3/4] memtx: allow quota overuse for truncation and deletion

[Ilya Kosarev]

Trying to perform space:truncate() and space:delete() while reaching
memtx_memory limit we could experience slab allocator failure. This
behavior seems to be quite surprising for users. Now we are allowing
to overuse memtx quota if needed for truncation or deletion, using flag
in struct quota. After performing truncation or deletion we reset the
flag so that quota can't be overused any more.

Closes #3807
---
 src/box/box.cc        | 27 ++++++++++++++++++++++++++-
 src/box/memtx_space.c | 18 ++++++++++++++----
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 1b2b27d61..1a63f25a2 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1321,9 +1321,34 @@ space_truncate(struct space *space)
 	ops_buf_end = mp_encode_uint(ops_buf_end, 1);
 	assert(ops_buf_end < buf + buf_size);
 
+	struct txn *txn = NULL;
+	struct txn_savepoint *txn_svp = NULL;
+	if (!box_txn()) {
+		txn = txn_begin();
+		if (txn == NULL)
+			diag_raise();
+	} else {
+		txn_svp = box_txn_savepoint();
+		if (txn_svp == NULL)
+			diag_raise();
+	}
+	struct space *truncate_space = space_cache_find_xc(BOX_TRUNCATE_ID);
+	quota_on_off(&((struct memtx_engine *)truncate_space->engine)->quota, false);
 	if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end,
-		       ops_buf, ops_buf_end, 0, NULL) != 0)
+		       ops_buf, ops_buf_end, 0, NULL) != 0) {
+		quota_on_off(&((struct memtx_engine *)truncate_space->engine)->quota, true);
+		if (txn != NULL)
+			txn_rollback(txn);
+		else
+			box_txn_rollback_to_savepoint(txn_svp);
+		fiber_gc();
 		diag_raise();
+	}
+	quota_on_off(&((struct memtx_engine *)truncate_space->engine)->quota, true);
+	if (txn != NULL && txn_commit(txn) != 0) {
+		fiber_gc();
+		diag_raise();
+	}
 }
 
 int
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 6ef84e045..3542450f7 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -28,6 +28,7 @@
  * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  */
+#include "exception.h"
 #include "memtx_space.h"
 #include "space.h"
 #include "iproto_constants.h"
@@ -354,10 +355,19 @@ memtx_space_execute_delete(struct space *space, struct txn *txn,
 	struct tuple *old_tuple;
 	if (index_get(pk, key, part_count, &old_tuple) != 0)
 		return -1;
-	if (old_tuple != NULL &&
-	    memtx_space->replace(space, old_tuple, NULL,
-				 DUP_REPLACE_OR_INSERT, &stmt->old_tuple) != 0)
-		return -1;
+	if (old_tuple != NULL) {
+		bool oom = !diag_is_empty(diag_get()) &&
+			   diag_last_error(diag_get())->type == &type_OutOfMemory;
+		if (oom)
+			quota_on_off(&((struct memtx_engine *)space->engine)->quota, false);
+		int rc = memtx_space->replace(space, old_tuple, NULL,
+					      DUP_REPLACE_OR_INSERT,
+					      &stmt->old_tuple);
+		if (oom)
+			quota_on_off(&((struct memtx_engine *)space->engine)->quota, true);
+		if (rc != 0)
+			return -1;
+	}
 	stmt->engine_savepoint = stmt;
 	*result = stmt->old_tuple;
 	return 0;
-- 
2.17.1

[Tarantool-patches] [PATCH v4 4/4] test: add tests for truncation and deletion

[Ilya Kosarev]

Trying to perform space:truncate() and space:delete() while reaching
memtx_memory limit we could experience slab allocator failure. Now it
is solved through quota overuse tech. This commit introduces
corresponding tests.

Part of #3807
---
 src/box/memtx_space.c                |  15 +++-
 src/lib/core/errinj.h                |   1 +
 test/box/errinj.result               |   1 +
 test/engine/engine.cfg               |   6 ++
 test/engine/low_memory.lua           |   8 ++
 test/engine/stress_delete.result     | 111 +++++++++++++++++++++++++++
 test/engine/stress_delete.test.lua   |  55 +++++++++++++
 test/engine/stress_truncate.result   | 103 +++++++++++++++++++++++++
 test/engine/stress_truncate.test.lua |  52 +++++++++++++
 9 files changed, 349 insertions(+), 3 deletions(-)
 create mode 100644 test/engine/low_memory.lua
 create mode 100644 test/engine/stress_delete.result
 create mode 100644 test/engine/stress_delete.test.lua
 create mode 100644 test/engine/stress_truncate.result
 create mode 100644 test/engine/stress_truncate.test.lua

diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 3542450f7..2ce09018d 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -261,9 +261,18 @@ memtx_space_replace_all_keys(struct space *space, struct tuple *old_tuple,
 	 * Ensure we have enough slack memory to guarantee
 	 * successful statement-level rollback.
 	 */
-	if (memtx_index_extent_reserve(memtx, new_tuple != NULL ?
-				       RESERVE_EXTENTS_BEFORE_REPLACE :
-				       RESERVE_EXTENTS_BEFORE_DELETE) != 0)
+	int reserve_extents_num = new_tuple != NULL ?
+				  RESERVE_EXTENTS_BEFORE_REPLACE :
+				  RESERVE_EXTENTS_BEFORE_DELETE;
+	ERROR_INJECT(ERRINJ_RESERVE_EXTENTS_BEFORE_DELETE, {
+		/**
+		 * Set huge number of needed reserved extents to
+		 * provoke delete failure.
+		 */
+		if (new_tuple == NULL)
+			reserve_extents_num = memtx->num_reserved_extents + 1;
+	});
+	if (memtx_index_extent_reserve(memtx, reserve_extents_num) != 0)
 		return -1;
 
 	uint32_t i = 0;
diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
index 672da2119..2d3331cfa 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_RESERVE_EXTENTS_BEFORE_DELETE, 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 f043c6689..1f99470e3 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -63,6 +63,7 @@ evals
   - ERRINJ_RELAY_SEND_DELAY: false
   - ERRINJ_RELAY_TIMEOUT: 0
   - ERRINJ_REPLICA_JOIN_DELAY: false
+  - ERRINJ_RESERVE_EXTENTS_BEFORE_DELETE: false
   - ERRINJ_SIO_READ_MAX: -1
   - ERRINJ_SNAP_COMMIT_DELAY: false
   - ERRINJ_SNAP_WRITE_DELAY: false
diff --git a/test/engine/engine.cfg b/test/engine/engine.cfg
index f1e7de274..c8c6fb130 100644
--- a/test/engine/engine.cfg
+++ b/test/engine/engine.cfg
@@ -5,6 +5,12 @@
     },
     "func_index.test.lua": {
         "memtx": {"engine": "memtx"}
+     },
+    "stress_delete.test.lua": {
+        "memtx": {"engine": "memtx"}
+     },
+    "stress_truncate.test.lua": {
+        "memtx": {"engine": "memtx"}
      }
 }
 
diff --git a/test/engine/low_memory.lua b/test/engine/low_memory.lua
new file mode 100644
index 000000000..46fd26d1b
--- /dev/null
+++ b/test/engine/low_memory.lua
@@ -0,0 +1,8 @@
+#!/usr/bin/env tarantool
+os = require('os')
+box.cfg({
+    listen              = os.getenv("LISTEN"),
+    memtx_memory        = 32 * 1024 * 1024,
+})
+
+require('console').listen(os.getenv('ADMIN'))
diff --git a/test/engine/stress_delete.result b/test/engine/stress_delete.result
new file mode 100644
index 000000000..bf92c780b
--- /dev/null
+++ b/test/engine/stress_delete.result
@@ -0,0 +1,111 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+
+test_run:cmd("create server master with script='engine/low_memory.lua'")
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server master')
+ | ---
+ | - true
+ | ...
+test_run:cmd("switch master")
+ | ---
+ | - true
+ | ...
+
+
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+function create_space(name)
+    local space = box.schema.create_space(name)
+    space:format({
+        { name = "id",  type = "unsigned" },
+        { name = "val", type = "str" }
+    })
+    space:create_index('primary', { parts = { 'id' } })
+    return space
+end;
+ | ---
+ | ...
+
+function insert(space, i)
+    space:insert({ i, string.rep(string.char(32 + math.random(127-32)), math.random(1024)) })
+end;
+ | ---
+ | ...
+
+function fill_space(space, start)
+    local _, err = nil
+    local i = start
+    while err == nil do _, err = pcall(insert, space, i) i = i + 1 end
+end;
+ | ---
+ | ...
+
+function stress_deletion(i, spaces)
+    local res, space = pcall(create_space, 'test' .. tostring(i))
+    if res then spaces[i] = space return end
+    fill_space(box.space.test, box.space.test:len())
+    for _, s in pairs(spaces) do fill_space(s, s:len()) end
+    box.space.test:delete(box.space.test:len() - 1)
+end;
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+
+_ = create_space('test')
+ | ---
+ | ...
+for i = 0, 27000 do insert(box.space.test, i) end
+ | ---
+ | ...
+
+spaces = {}
+ | ---
+ | ...
+counter = 0
+ | ---
+ | ...
+status = true
+ | ---
+ | ...
+res = nil
+ | ---
+ | ...
+errinj = box.error.injection
+ | ---
+ | ...
+errinj.set('ERRINJ_RESERVE_EXTENTS_BEFORE_DELETE', true)
+ | ---
+ | - ok
+ | ...
+while counter < 1400 do status, res = pcall(stress_deletion, counter, spaces) counter = counter + 1 end
+ | ---
+ | ...
+status
+ | ---
+ | - true
+ | ...
+res
+ | ---
+ | - null
+ | ...
+
+-- Cleanup.
+test_run:cmd('switch default')
+ | ---
+ | - true
+ | ...
+test_run:drop_cluster({'master'})
+ | ---
+ | ...
diff --git a/test/engine/stress_delete.test.lua b/test/engine/stress_delete.test.lua
new file mode 100644
index 000000000..b9014ece5
--- /dev/null
+++ b/test/engine/stress_delete.test.lua
@@ -0,0 +1,55 @@
+test_run = require('test_run').new()
+
+
+test_run:cmd("create server master with script='engine/low_memory.lua'")
+test_run:cmd('start server master')
+test_run:cmd("switch master")
+
+
+test_run:cmd("setopt delimiter ';'")
+function create_space(name)
+    local space = box.schema.create_space(name)
+    space:format({
+        { name = "id",  type = "unsigned" },
+        { name = "val", type = "str" }
+    })
+    space:create_index('primary', { parts = { 'id' } })
+    return space
+end;
+
+function insert(space, i)
+    space:insert({ i, string.rep(string.char(32 + math.random(127-32)), math.random(1024)) })
+end;
+
+function fill_space(space, start)
+    local _, err = nil
+    local i = start
+    while err == nil do _, err = pcall(insert, space, i) i = i + 1 end
+end;
+
+function stress_deletion(i, spaces)
+    local res, space = pcall(create_space, 'test' .. tostring(i))
+    if res then spaces[i] = space return end
+    fill_space(box.space.test, box.space.test:len())
+    for _, s in pairs(spaces) do fill_space(s, s:len()) end
+    box.space.test:delete(box.space.test:len() - 1)
+end;
+test_run:cmd("setopt delimiter ''");
+
+
+_ = create_space('test')
+for i = 0, 27000 do insert(box.space.test, i) end
+
+spaces = {}
+counter = 0
+status = true
+res = nil
+errinj = box.error.injection
+errinj.set('ERRINJ_RESERVE_EXTENTS_BEFORE_DELETE', true)
+while counter < 1400 do status, res = pcall(stress_deletion, counter, spaces) counter = counter + 1 end
+status
+res
+
+-- Cleanup.
+test_run:cmd('switch default')
+test_run:drop_cluster({'master'})
diff --git a/test/engine/stress_truncate.result b/test/engine/stress_truncate.result
new file mode 100644
index 000000000..3e2573021
--- /dev/null
+++ b/test/engine/stress_truncate.result
@@ -0,0 +1,103 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+
+test_run:cmd("create server master with script='engine/low_memory.lua'")
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server master')
+ | ---
+ | - true
+ | ...
+test_run:cmd("switch master")
+ | ---
+ | - true
+ | ...
+
+
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+function create_space(name)
+    local space = box.schema.create_space(name)
+    space:format({
+        { name = "id",  type = "unsigned" },
+        { name = "val", type = "str" }
+    })
+    space:create_index('primary', { parts = { 'id' } })
+    return space
+end;
+ | ---
+ | ...
+
+function insert(space, i)
+    space:insert({ i, string.rep(string.char(32 + math.random(127-32)), math.random(1024)) })
+end;
+ | ---
+ | ...
+
+function fill_space(space, start)
+    local _, err = nil
+    local i = start
+    while err == nil do _, err = pcall(insert, space, i) i = i + 1 end
+end;
+ | ---
+ | ...
+
+function stress_truncation(i, spaces)
+    local res, space = pcall(create_space, 'test' .. tostring(i))
+    if res then spaces[i] = space return end
+    fill_space(box.space.test, box.space.test:len())
+    for _, s in pairs(spaces) do s:truncate() end
+end;
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+
+_ = create_space('test')
+ | ---
+ | ...
+for i = 0, 27000 do insert(box.space.test, i) end
+ | ---
+ | ...
+
+spaces = {}
+ | ---
+ | ...
+counter = 0
+ | ---
+ | ...
+status = true
+ | ---
+ | ...
+res = nil
+ | ---
+ | ...
+while counter < 1400 do status, res = pcall(stress_truncation, counter, spaces) counter = counter + 1 end
+ | ---
+ | ...
+status
+ | ---
+ | - true
+ | ...
+res
+ | ---
+ | - null
+ | ...
+
+-- Cleanup.
+test_run:cmd('switch default')
+ | ---
+ | - true
+ | ...
+test_run:drop_cluster({'master'})
+ | ---
+ | ...
diff --git a/test/engine/stress_truncate.test.lua b/test/engine/stress_truncate.test.lua
new file mode 100644
index 000000000..83abd3d11
--- /dev/null
+++ b/test/engine/stress_truncate.test.lua
@@ -0,0 +1,52 @@
+test_run = require('test_run').new()
+
+
+test_run:cmd("create server master with script='engine/low_memory.lua'")
+test_run:cmd('start server master')
+test_run:cmd("switch master")
+
+
+test_run:cmd("setopt delimiter ';'")
+function create_space(name)
+    local space = box.schema.create_space(name)
+    space:format({
+        { name = "id",  type = "unsigned" },
+        { name = "val", type = "str" }
+    })
+    space:create_index('primary', { parts = { 'id' } })
+    return space
+end;
+
+function insert(space, i)
+    space:insert({ i, string.rep(string.char(32 + math.random(127-32)), math.random(1024)) })
+end;
+
+function fill_space(space, start)
+    local _, err = nil
+    local i = start
+    while err == nil do _, err = pcall(insert, space, i) i = i + 1 end
+end;
+
+function stress_truncation(i, spaces)
+    local res, space = pcall(create_space, 'test' .. tostring(i))
+    if res then spaces[i] = space return end
+    fill_space(box.space.test, box.space.test:len())
+    for _, s in pairs(spaces) do s:truncate() end
+end;
+test_run:cmd("setopt delimiter ''");
+
+
+_ = create_space('test')
+for i = 0, 27000 do insert(box.space.test, i) end
+
+spaces = {}
+counter = 0
+status = true
+res = nil
+while counter < 1400 do status, res = pcall(stress_truncation, counter, spaces) counter = counter + 1 end
+status
+res
+
+-- Cleanup.
+test_run:cmd('switch default')
+test_run:drop_cluster({'master'})
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v4 0/4] Safe truncation and deletion

[Konstantin Osipov]

* Ilya Kosarev <i.kosarev@tarantool.org> [20/02/14 22:41]:
> Changes in v2:
>   Approach changed completely: now we are not trying to allocate
>   service tuples in some safe way, but increasing memtx quota so
>   that space:truncate() and space:delete() won't fail on allocation.


> 
> Changes in v3:
>   Now we are not increasing memtx quota. Instead we just set a flag to
>   allow quota overuse for space:truncate() and space:delete().
> 
> Changes in v4:
>   improved quota on/off flag style
>   took into account fail cases for box_upsert in space_truncate
>   now we are switching the quota off for deletion only in case there is
>   OutOfMemory error in diag
>   removed extra checks
>   added tests

Is it intentional that first you discuss the
recommendation I make on the mailing list, and then silently
disregard it?

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v4 0/4] Safe truncation and deletion

[Ilya Kosarev]

[-- Attachment #1: Type: text/plain, Size: 1281 bytes --]


I sent 2 versions of this patchset. Here is the second one
with discussed approach:
https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/014191.html
And tarantool/small patch:
https://lists.tarantool.org/pipermail/tarantool-patches/2020-February/014190.html 
>Суббота, 15 февраля 2020, 0:00 +03:00 от Konstantin Osipov <kostja.osipov@gmail.com>:
> 
>* Ilya Kosarev < i.kosarev@tarantool.org > [20/02/14 22:41]:
>> Changes in v2:
>> Approach changed completely: now we are not trying to allocate
>> service tuples in some safe way, but increasing memtx quota so
>> that space:truncate() and space:delete() won't fail on allocation.
>
>
>>
>> Changes in v3:
>> Now we are not increasing memtx quota. Instead we just set a flag to
>> allow quota overuse for space:truncate() and space:delete().
>>
>> Changes in v4:
>> improved quota on/off flag style
>> took into account fail cases for box_upsert in space_truncate
>> now we are switching the quota off for deletion only in case there is
>> OutOfMemory error in diag
>> removed extra checks
>> added tests
>
>Is it intentional that first you discuss the
>recommendation I make on the mailing list, and then silently
>disregard it?
>
>--
>Konstantin Osipov, Moscow, Russia 
 
 
--
Ilya Kosarev
 

[-- Attachment #2: Type: text/html, Size: 2004 bytes --]

Re: [Tarantool-patches] [PATCH v4 2/4] b-tree: return NULL on matras_alloc fail

[Vladislav Shpilevoy]

Thanks for the patch!

See 3 comments below.

On 14/02/2020 20:39, Ilya Kosarev wrote:
> In bps_tree_create_leaf we use matras_alloc in case

1. This is not only about bps_tree_create_leaf() anymore. You
also patched bps_tree_create_inner().

> bps_tree_garbage_pop didn't work out. However it also might not
> succeed. Then we need to return NULL instead of dereferencing NULL
> pointer.
> 
> Part of #3807
> ---
>  src/lib/salad/bps_tree.h | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/src/lib/salad/bps_tree.h b/src/lib/salad/bps_tree.h
> index d28b53f53..d71340143 100644
> --- a/src/lib/salad/bps_tree.h
> +++ b/src/lib/salad/bps_tree.h
> @@ -3710,6 +3718,8 @@ bps_tree_process_insert_leaf(struct bps_tree *tree,
>  		bps_tree_block_id_t new_root_id = (bps_tree_block_id_t)(-1);
>  		struct bps_inner *new_root = bps_tree_create_inner(tree,
>  				&new_root_id);
> +		if (!new_root)
> +			return -1;

2. Here are two problems.

1) This can't fail, because above there is bps_tree_reserve_blocks()
  invocation. Looks like it reserves enough blocks (but I propose you
  to check it, I don't know).

2) Even if that fails, the tree to that moment is already partially
  changed. And if you return -1 here, it is corrupted. To test that I
  added an error injection to bps_tree_process_insert_leaf() in this
  place. Initially it was false. I called box.cfg{}. Then I set it
  true. And created a space, an index. Tried to insert 100 numbers.
  It failed due to OOM. Then I turned the errinj off again, inserted
  the values again, and made a select. I got broken order of the
  select, and values returned multiple times.

Here is what I have done:

================================================================================
--- a/src/lib/salad/bps_tree.h
+++ b/src/lib/salad/bps_tree.h
@@ -38,6 +38,8 @@
 #include <stdio.h> /* printf */
 #include "small/matras.h"
 
+extern int bps_errinj;
+
 /* {{{ BPS-tree description */
 /**
  * BPS-tree implementation.
@@ -3716,6 +3718,8 @@ bps_tree_process_insert_leaf(struct bps_tree *tree,
                                leaf_path_elem, &new_path_elem, mc1, new_elem);
 
                bps_tree_block_id_t new_root_id = (bps_tree_block_id_t)(-1);
+               if (bps_errinj)
+                       return -1;
                struct bps_inner *new_root = bps_tree_create_inner(tree,
                                &new_root_id);
                if (!new_root)
================================================================================
Also you need to declare bps_errinj somewhere in a .c file.

Here is the test:

    box.cfg{}

    -- In lldb/gdb make bps_errinj variable 1.
    s = box.schema.create_space('test')
    _ = s:create_index('pk')
    for i = 1, 100 do s:replace{i} end

    -- This was an error. Now make bps_errinj 0 again.
    for i = 1, 100 do s:replace{i} end
    s:select{}

The select will return tuples in a wrong order.

>  		new_root->header.size = 2;
>  		new_root->child_ids[0] = tree->root_id;
>  		new_root->child_ids[1] = new_block_id;
> @@ -4030,6 +4042,8 @@ bps_tree_process_insert_inner(struct bps_tree *tree,
>  		bps_tree_block_id_t new_root_id = (bps_tree_block_id_t)(-1);
>  		struct bps_inner *new_root =
>  			bps_tree_create_inner(tree, &new_root_id);
> +		if (!new_root)
> +			return -1;

3. The same problem is here. You can't return from now on, the
tree is changed already.

>  		new_root->header.size = 2;
>  		new_root->child_ids[0] = tree->root_id;
>  		new_root->child_ids[1] = new_block_id;
> 

Re: [Tarantool-patches] [PATCH v4 4/4] test: add tests for truncation and deletion

[Vladislav Shpilevoy]

Thanks for the patch!

Please, put tests to the same commit as the patch.

See 10 comments below.

On 14/02/2020 20:39, Ilya Kosarev wrote:
> Trying to perform space:truncate() and space:delete() while reaching
> memtx_memory limit we could experience slab allocator failure. Now it
> is solved through quota overuse tech. This commit introduces
> corresponding tests.
> 
> Part of #3807
> ---
>  src/box/memtx_space.c                |  15 +++-
>  src/lib/core/errinj.h                |   1 +
>  test/box/errinj.result               |   1 +
>  test/engine/engine.cfg               |   6 ++
>  test/engine/low_memory.lua           |   8 ++
>  test/engine/stress_delete.result     | 111 +++++++++++++++++++++++++++
>  test/engine/stress_delete.test.lua   |  55 +++++++++++++
>  test/engine/stress_truncate.result   | 103 +++++++++++++++++++++++++
>  test/engine/stress_truncate.test.lua |  52 +++++++++++++
>  9 files changed, 349 insertions(+), 3 deletions(-)
>  create mode 100644 test/engine/low_memory.lua
>  create mode 100644 test/engine/stress_delete.result
>  create mode 100644 test/engine/stress_delete.test.lua
>  create mode 100644 test/engine/stress_truncate.result
>  create mode 100644 test/engine/stress_truncate.test.lua
> 
> diff --git a/test/engine/engine.cfg b/test/engine/engine.cfg
> index f1e7de274..c8c6fb130 100644
> --- a/test/engine/engine.cfg
> +++ b/test/engine/engine.cfg
> @@ -5,6 +5,12 @@
>      },
>      "func_index.test.lua": {
>          "memtx": {"engine": "memtx"}
> +     },
> +    "stress_delete.test.lua": {
> +        "memtx": {"engine": "memtx"}
> +     },
> +    "stress_truncate.test.lua": {
> +        "memtx": {"engine": "memtx"}
>       }

1. This folder (engine/) is supposed to have only
tests running on both engines. It is purpose of this
folder. If you need something to run for one engine only,
please, add it to a different folder. For you case the
box/ folder is ok.

>  }
>  
> diff --git a/test/engine/low_memory.lua b/test/engine/low_memory.lua
> new file mode 100644
> index 000000000..46fd26d1b
> --- /dev/null
> +++ b/test/engine/low_memory.lua
> @@ -0,0 +1,8 @@
> +#!/usr/bin/env tarantool
> +os = require('os')
> +box.cfg({
> +    listen              = os.getenv("LISTEN"),
> +    memtx_memory        = 32 * 1024 * 1024,
> +})
> +
> +require('console').listen(os.getenv('ADMIN'))
> diff --git a/test/engine/stress_delete.result b/test/engine/stress_delete.result
> new file mode 100644
> index 000000000..bf92c780b
> --- /dev/null
> +++ b/test/engine/stress_delete.result

2. Since this is a test for a bug, and I would say quite a
big test, it should follow the convention and have the
corresponding name pattern: 'gh-####-short-description.test.lua'.

3. Please, don't split test of that bug into separate files.
You just duplicated lots of the code.

4. The test uses errinj, but is not added to release_disabled.
Please, add.

> @@ -0,0 +1,111 @@
> +-- test-run result file version 2
> +test_run = require('test_run').new()
> + | ---
> + | ...

5. When a test is related to a ticket (which is nearly always),
you need to write a comment saying ticket number and its
description. See examples in other test files.

> +
> +
> +test_run:cmd("create server master with script='engine/low_memory.lua'")
> + | ---
> + | - true
> + | ...
> +test_run:cmd('start server master')
> + | ---
> + | - true
> + | ...
> +test_run:cmd("switch master")
> + | ---
> + | - true
> + | ...
> +
> +
> +test_run:cmd("setopt delimiter ';'")
> + | ---
> + | - true
> + | ...
> +function create_space(name)
> +    local space = box.schema.create_space(name)
> +    space:format({
> +        { name = "id",  type = "unsigned" },
> +        { name = "val", type = "str" }
> +    })
> +    space:create_index('primary', { parts = { 'id' } })
> +    return space
> +end;
> + | ---
> + | ...
> +
> +function insert(space, i)
> +    space:insert({ i, string.rep(string.char(32 + math.random(127-32)), math.random(1024)) })
> +end;

6. Why does it matter whether data is random? IMO, on the
contrary, it reduces reproducibility.

> + | ---
> + | ...
> +
> +function fill_space(space, start)
> +    local _, err = nil
> +    local i = start
> +    while err == nil do _, err = pcall(insert, space, i) i = i + 1 end
> +end;
> + | ---
> + | ...
> +
> +function stress_deletion(i, spaces)
> +    local res, space = pcall(create_space, 'test' .. tostring(i))
> +    if res then spaces[i] = space return end
> +    fill_space(box.space.test, box.space.test:len())
> +    for _, s in pairs(spaces) do fill_space(s, s:len()) end
> +    box.space.test:delete(box.space.test:len() - 1)
> +end;
> + | ---
> + | ...
> +test_run:cmd("setopt delimiter ''");
> + | ---
> + | - true
> + | ...
> +
> +
> +_ = create_space('test')
> + | ---
> + | ...
> +for i = 0, 27000 do insert(box.space.test, i) end

7. Why exactly 27k?

8. Seems like drop of this space 'test' does not change
anything. I dropped it and the test passed.

> + | ---
> + | ...
> +
> +spaces = {}
> + | ---
> + | ...
> +counter = 0
> + | ---
> + | ...
> +status = true
> + | ---
> + | ...
> +res = nil
> + | ---
> + | ...
> +errinj = box.error.injection
> + | ---
> + | ...
> +errinj.set('ERRINJ_RESERVE_EXTENTS_BEFORE_DELETE', true)

9. I dropped that error injection and nothing changed in the
output. What do you need it for?

> + | ---
> + | - ok
> + | ...
> +while counter < 1400 do status, res = pcall(stress_deletion, counter, spaces) counter = counter + 1 end

10. Why 1400?

> + | ---
> + | ...
> +status
> + | ---
> + | - true
> + | ...
> +res
> + | ---
> + | - null
> + | ...

I have more comments, but I will keep them until I understand what is
happening in these tests.

Re: [Tarantool-patches] [PATCH v4 3/4] memtx: allow quota overuse for truncation and deletion

[Vladislav Shpilevoy]

Thanks for the patch!

See 7 comments below.

On 14/02/2020 20:39, Ilya Kosarev wrote:
> Trying to perform space:truncate() and space:delete() while reaching
> memtx_memory limit we could experience slab allocator failure. This
> behavior seems to be quite surprising for users. Now we are allowing
> to overuse memtx quota if needed for truncation or deletion, using flag
> in struct quota. After performing truncation or deletion we reset the
> flag so that quota can't be overused any more.
> 
> Closes #3807
> ---
>  src/box/box.cc        | 27 ++++++++++++++++++++++++++-
>  src/box/memtx_space.c | 18 ++++++++++++++----
>  2 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 1b2b27d61..1a63f25a2 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1321,9 +1321,34 @@ space_truncate(struct space *space)
>  	ops_buf_end = mp_encode_uint(ops_buf_end, 1);
>  	assert(ops_buf_end < buf + buf_size);

1. Please, add a comment why do you open a transaction.

> +	struct txn *txn = NULL;
> +	struct txn_savepoint *txn_svp = NULL;
> +	if (!box_txn()) {
> +		txn = txn_begin();
> +		if (txn == NULL)
> +			diag_raise();
> +	} else {
> +		txn_svp = box_txn_savepoint();
> +		if (txn_svp == NULL)
> +			diag_raise();
> +	}
> +	struct space *truncate_space = space_cache_find_xc(BOX_TRUNCATE_ID);
> +	quota_on_off(&((struct memtx_engine *)truncate_space->engine)->quota, false);

2. This line is out of 80 symbols. This is a very simple rule. If a line
is longer than 80 symbols, then it is not correct. Please, customize your
text editor. Every editor has an option to show so called 'rulers' on
certain columns. I set mine to 80, 66, and 50 for commit titles. Then you
will see too long lines before sending a patch. This helps a lot.

3. You repeat this super long thing 3 times:

    &((struct memtx_engine *)truncate_space->engine)->quota

in one function. Wouldn't it be better to save it once into a
'struct quota *' variable and use it?

>  	if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end,
> -		       ops_buf, ops_buf_end, 0, NULL) != 0)
> +		       ops_buf, ops_buf_end, 0, NULL) != 0) {
> +		quota_on_off(&((struct memtx_engine *)truncate_space->engine)->quota, true);

4. Now when I see quota_on_off() in action I think the name is
really confusing. For example:

    quota_on_off(true)

What does it mean? True means 'on' or 'off'? I propose to make two
functions: quota_enable() and quota_disable(), with one
'struct quota *' argument.

> +		if (txn != NULL)
> +			txn_rollback(txn);
> +		else
> +			box_txn_rollback_to_savepoint(txn_svp);
> +		fiber_gc();
>  		diag_raise();
> +	}
> +	quota_on_off(&((struct memtx_engine *)truncate_space->engine)->quota, true);
> +	if (txn != NULL && txn_commit(txn) != 0) {
> +		fiber_gc();
> +		diag_raise();
> +	}
>  }
>  
>  int
> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
> index 6ef84e045..3542450f7 100644
> --- a/src/box/memtx_space.c
> +++ b/src/box/memtx_space.c
> @@ -28,6 +28,7 @@
>   * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>   * SUCH DAMAGE.
>   */
> +#include "exception.h"
>  #include "memtx_space.h"
>  #include "space.h"
>  #include "iproto_constants.h"
> @@ -354,10 +355,19 @@ memtx_space_execute_delete(struct space *space, struct txn *txn,
>  	struct tuple *old_tuple;
>  	if (index_get(pk, key, part_count, &old_tuple) != 0)
>  		return -1;
> -	if (old_tuple != NULL &&
> -	    memtx_space->replace(space, old_tuple, NULL,
> -				 DUP_REPLACE_OR_INSERT, &stmt->old_tuple) != 0)
> -		return -1;
> +	if (old_tuple != NULL) {
> +		bool oom = !diag_is_empty(diag_get()) &&
> +			   diag_last_error(diag_get())->type == &type_OutOfMemory;

5. Please, rename to 'is_oom'. For flags we always use 'is_/has_/...'
prefix. Except some places, where it was missed at review stage.

6. This line is also out of 80 symbols. And two lines below about
quota enabling/disabling.

7. I am not sure what is happening here. You check for 'oom'
*before* doing actual change? Why?! An oom can happen only below,
in memtx_space->replace().

> +		if (oom)
> +			quota_on_off(&((struct memtx_engine *)space->engine)->quota, false);
> +		int rc = memtx_space->replace(space, old_tuple, NULL,
> +					      DUP_REPLACE_OR_INSERT,
> +					      &stmt->old_tuple);
> +		if (oom)
> +			quota_on_off(&((struct memtx_engine *)space->engine)->quota, true);
> +		if (rc != 0)
> +			return -1;
> +	}
>  	stmt->engine_savepoint = stmt;
>  	*result = stmt->old_tuple;
>  	return 0;