[Tarantool-patches] [PATCH] memtx: fix tuples references on concurrent replaces

[Tarantool-patches] [PATCH] memtx: fix tuples references on concurrent replaces

[Ilya Kosarev]

Since 527b02a2ee6a9a205d8e2c8f38bbb84edf0d6557 (memtx: add yields
during index build) memtx_build_on_replace was introduced to handle
concurrent updates. The problem here was that the tuples being handled
with this trigger did not get reference counter promotion, leading to a
number of wrong behavior cases. Now this problem is solved.
This problem was found through primary index altering with updates in
background fiber. Corresponding test is introduced.

Closes #4973
---
Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4973-segfaults-on-alter
Issue: https://github.com/tarantool/tarantool/issues/4973

@ChangeLog:
 * Fix concurrent replaces on index building. Tuples are now referenced
 on all needed execution paths.

 src/box/memtx_space.c                         |  8 +-
 test/engine/engine.cfg                        |  5 +-
 .../gh-4973-concurrent-alter-fails.result     | 74 +++++++++++++++++++
 .../gh-4973-concurrent-alter-fails.test.lua   | 32 ++++++++
 4 files changed, 117 insertions(+), 2 deletions(-)
 create mode 100644 test/engine/gh-4973-concurrent-alter-fails.result
 create mode 100644 test/engine/gh-4973-concurrent-alter-fails.test.lua

diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 09f0de4cec..8755920266 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -975,10 +975,16 @@ memtx_build_on_replace(struct trigger *trigger, void *event)
 						    DUP_REPLACE_OR_INSERT;
 	state->rc = index_replace(state->index, stmt->old_tuple,
 				  stmt->new_tuple, mode, &delete);
-
 	if (state->rc != 0) {
 		diag_move(diag_get(), &state->diag);
+		return 0;
 	}
+	/*
+	 * All tuples stored in a memtx space must be
+	 * referenced by the primary index.
+	 */
+	if (state->index->def->iid == 0 && stmt->new_tuple != NULL)
+		tuple_ref(stmt->new_tuple);
 	return 0;
 }
 
diff --git a/test/engine/engine.cfg b/test/engine/engine.cfg
index f1e7de2745..f017a03ec2 100644
--- a/test/engine/engine.cfg
+++ b/test/engine/engine.cfg
@@ -5,7 +5,10 @@
     },
     "func_index.test.lua": {
         "memtx": {"engine": "memtx"}
-     }
+    },
+    "gh-4973-concurrent-alter-fails.test.lua": {
+        "memtx": {"engine": "memtx"}
+    }
 }
 
 
diff --git a/test/engine/gh-4973-concurrent-alter-fails.result b/test/engine/gh-4973-concurrent-alter-fails.result
new file mode 100644
index 0000000000..1a6624a479
--- /dev/null
+++ b/test/engine/gh-4973-concurrent-alter-fails.result
@@ -0,0 +1,74 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+errinj = box.error.injection
+ | ---
+ | ...
+
+space = box.schema.space.create('gh-4973-alter', {engine = 'memtx'})
+ | ---
+ | ...
+space:format({ {'key', 'unsigned'}, {'value', 'string'}, {'key_new', 'unsigned'} })
+ | ---
+ | ...
+index = space:create_index('primary', {parts = {'key'}})
+ | ---
+ | ...
+
+N = 10000
+ | ---
+ | ...
+value = string.rep('a', 10)
+ | ---
+ | ...
+box.atomic(function() for i = 1, N do space:insert({i, value, i}) end end)
+ | ---
+ | ...
+
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+function random_update()
+    local x = space.index.primary:random(math.random(N))
+    local op = math.random(10)
+    if op < 10 then space:update({x[1]}, {{'=', 2, string.rep('b', 10)}}) end
+    if op == 10 then space:delete({x[1]}) end
+end;
+ | ---
+ | ...
+
+finished_updates = false;
+ | ---
+ | ...
+fiber = require('fiber');
+ | ---
+ | ...
+updater = fiber.create(function()
+    for _ = 1, N do random_update() end
+    finished_updates = true
+end)
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | ...
+
+space.index.primary:alter({parts = {'key_new'}})
+ | ---
+ | ...
+errinj.set('ERRINJ_BUILD_INDEX_DELAY', true)
+ | ---
+ | - ok
+ | ...
+test_run:wait_cond(function() return finished_updates end, 5)
+ | ---
+ | - true
+ | ...
+errinj.set('ERRINJ_BUILD_INDEX_DELAY', false)
+ | ---
+ | - ok
+ | ...
+box.snapshot()
+ | ---
+ | - ok
+ | ...
diff --git a/test/engine/gh-4973-concurrent-alter-fails.test.lua b/test/engine/gh-4973-concurrent-alter-fails.test.lua
new file mode 100644
index 0000000000..4fd1a848d4
--- /dev/null
+++ b/test/engine/gh-4973-concurrent-alter-fails.test.lua
@@ -0,0 +1,32 @@
+test_run = require('test_run').new()
+errinj = box.error.injection
+
+space = box.schema.space.create('gh-4973-alter', {engine = 'memtx'})
+space:format({ {'key', 'unsigned'}, {'value', 'string'}, {'key_new', 'unsigned'} })
+index = space:create_index('primary', {parts = {'key'}})
+
+N = 10000
+value = string.rep('a', 10)
+box.atomic(function() for i = 1, N do space:insert({i, value, i}) end end)
+
+test_run:cmd("setopt delimiter ';'")
+function random_update()
+    local x = space.index.primary:random(math.random(N))
+    local op = math.random(10)
+    if op < 10 then space:update({x[1]}, {{'=', 2, string.rep('b', 10)}}) end
+    if op == 10 then space:delete({x[1]}) end
+end;
+
+finished_updates = false;
+fiber = require('fiber');
+updater = fiber.create(function()
+    for _ = 1, N do random_update() end
+    finished_updates = true
+end)
+test_run:cmd("setopt delimiter ''");
+
+space.index.primary:alter({parts = {'key_new'}})
+errinj.set('ERRINJ_BUILD_INDEX_DELAY', true)
+test_run:wait_cond(function() return finished_updates end, 5)
+errinj.set('ERRINJ_BUILD_INDEX_DELAY', false)
+box.snapshot()
-- 
2.17.1

Re: [Tarantool-patches] [PATCH] memtx: fix tuples references on concurrent replaces

[Aleksandr Lyapunov]

Thanks for the patch! See my comment below.

On 6/7/20 5:08 PM, Ilya Kosarev wrote:
> +	/*
> +	 * All tuples stored in a memtx space must be
> +	 * referenced by the primary index.
> +	 */
> +	if (state->index->def->iid == 0)
> +		tuple_ref(stmt->new_tuple);
I'm afraid that stmt->new_tuple can be NULL here.
Could you please create another test for this issue when some tuples are 
deleted during snapshot. I bet it will crash.

[Tarantool-patches] [PATCH] memtx: fix tuples references on concurrent replaces

[Ilya Kosarev]

Since 527b02a2ee6a9a205d8e2c8f38bbb84edf0d6557 (memtx: add yields
during index build) memtx_build_on_replace was introduced to handle
concurrent updates. The problem here was that the tuples being handled
with this trigger did not get reference counter promotion, leading to a
number of wrong behavior cases. Now this problem is solved.
This problem was found through primary index altering with updates in
background fiber. Corresponding test is introduced.

Closes #4973
---
Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4973-segfaults-on-alter
Issue: https://github.com/tarantool/tarantool/issues/4973

@ChangeLog:
 * Fix concurrent replaces on index building. Tuples are now referenced
 on all needed execution paths.

 src/box/memtx_space.c                         |  8 ++-
 test/engine/engine.cfg                        |  5 +-
 ...-4973-segfaults-on-concurrent-alter.result | 72 +++++++++++++++++++
 ...973-segfaults-on-concurrent-alter.test.lua | 30 ++++++++
 4 files changed, 113 insertions(+), 2 deletions(-)
 create mode 100644 test/engine/gh-4973-segfaults-on-concurrent-alter.result
 create mode 100644 test/engine/gh-4973-segfaults-on-concurrent-alter.test.lua

diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 09f0de4cec..a1ec9d03ee 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -975,10 +975,16 @@ memtx_build_on_replace(struct trigger *trigger, void *event)
 						    DUP_REPLACE_OR_INSERT;
 	state->rc = index_replace(state->index, stmt->old_tuple,
 				  stmt->new_tuple, mode, &delete);
-
 	if (state->rc != 0) {
 		diag_move(diag_get(), &state->diag);
+		return 0;
 	}
+	/*
+	 * All tuples stored in a memtx space must be
+	 * referenced by the primary index.
+	 */
+	if (state->index->def->iid == 0)
+		tuple_ref(stmt->new_tuple);
 	return 0;
 }
 
diff --git a/test/engine/engine.cfg b/test/engine/engine.cfg
index f1e7de2745..0b09f7c1d2 100644
--- a/test/engine/engine.cfg
+++ b/test/engine/engine.cfg
@@ -5,7 +5,10 @@
     },
     "func_index.test.lua": {
         "memtx": {"engine": "memtx"}
-     }
+    },
+    "gh-4973-segfaults-on-concurrent-alter.test.lua": {
+        "memtx": {"engine": "memtx"}
+    }
 }
 
 
diff --git a/test/engine/gh-4973-segfaults-on-concurrent-alter.result b/test/engine/gh-4973-segfaults-on-concurrent-alter.result
new file mode 100644
index 0000000000..e0f7eec718
--- /dev/null
+++ b/test/engine/gh-4973-segfaults-on-concurrent-alter.result
@@ -0,0 +1,72 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+errinj = box.error.injection
+ | ---
+ | ...
+
+space = box.schema.space.create('gh-4973-alter', {engine = 'memtx'})
+ | ---
+ | ...
+space:format({ {'key', 'unsigned'}, {'value', 'string'}, {'key_new', 'unsigned'} })
+ | ---
+ | ...
+index = space:create_index('primary', {parts = {'key'}})
+ | ---
+ | ...
+
+N = 10000
+ | ---
+ | ...
+value = string.rep('a', 10)
+ | ---
+ | ...
+box.atomic(function() for i = 1, N do space:insert({i, value, i}) end end)
+ | ---
+ | ...
+
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+function random_update()
+    local x = space.index.primary:random(math.random(N))
+    space:update({x[1]}, {{'=', 2, string.rep('b', 10)}})
+end;
+ | ---
+ | ...
+
+finished_updates = false;
+ | ---
+ | ...
+fiber = require('fiber');
+ | ---
+ | ...
+updater = fiber.create(function()
+    for _ = 1, N / 10 do random_update() end
+    finished_updates = true
+end)
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | ...
+
+space.index.primary:alter({parts = {'key_new'}})
+ | ---
+ | ...
+errinj.set('ERRINJ_BUILD_INDEX_DELAY', true)
+ | ---
+ | - ok
+ | ...
+test_run:wait_cond(function() return finished_updates end, 2)
+ | ---
+ | - true
+ | ...
+errinj.set('ERRINJ_BUILD_INDEX_DELAY', false)
+ | ---
+ | - ok
+ | ...
+box.snapshot()
+ | ---
+ | - ok
+ | ...
diff --git a/test/engine/gh-4973-segfaults-on-concurrent-alter.test.lua b/test/engine/gh-4973-segfaults-on-concurrent-alter.test.lua
new file mode 100644
index 0000000000..ca26f540d3
--- /dev/null
+++ b/test/engine/gh-4973-segfaults-on-concurrent-alter.test.lua
@@ -0,0 +1,30 @@
+test_run = require('test_run').new()
+errinj = box.error.injection
+
+space = box.schema.space.create('gh-4973-alter', {engine = 'memtx'})
+space:format({ {'key', 'unsigned'}, {'value', 'string'}, {'key_new', 'unsigned'} })
+index = space:create_index('primary', {parts = {'key'}})
+
+N = 10000
+value = string.rep('a', 10)
+box.atomic(function() for i = 1, N do space:insert({i, value, i}) end end)
+
+test_run:cmd("setopt delimiter ';'")
+function random_update()
+    local x = space.index.primary:random(math.random(N))
+    space:update({x[1]}, {{'=', 2, string.rep('b', 10)}})
+end;
+
+finished_updates = false;
+fiber = require('fiber');
+updater = fiber.create(function()
+    for _ = 1, N / 10 do random_update() end
+    finished_updates = true
+end)
+test_run:cmd("setopt delimiter ''");
+
+space.index.primary:alter({parts = {'key_new'}})
+errinj.set('ERRINJ_BUILD_INDEX_DELAY', true)
+test_run:wait_cond(function() return finished_updates end, 2)
+errinj.set('ERRINJ_BUILD_INDEX_DELAY', false)
+box.snapshot()
-- 
2.17.1