[PATCH 1/2] vinyl: fix crash during index build

[PATCH 1/2] vinyl: fix crash during index build

[Vladimir Davydov]

To propagate changes applied to a space while a new index is being
built, we install an on_replace trigger. In case the on_replace
trigger callback fails, we abort the DDL operation.

The problem is the trigger may yield, e.g. to check the unique
constraint of the new index. This opens a time window for the DDL
operation to complete and clear the trigger. If this happens, the
trigger will try to access the outdated build context and crash:

 | #0  0x558f29cdfbc7 in print_backtrace+9
 | #1  0x558f29bd37db in _ZL12sig_fatal_cbiP9siginfo_tPv+1e7
 | #2  0x7fe24e4ab0e0 in __restore_rt+0
 | #3  0x558f29bfe036 in error_unref+1a
 | #4  0x558f29bfe0d1 in diag_clear+27
 | #5  0x558f29bfe133 in diag_move+1c
 | #6  0x558f29c0a4e2 in vy_build_on_replace+236
 | #7  0x558f29cf3554 in trigger_run+7a
 | #8  0x558f29c7b494 in txn_commit_stmt+125
 | #9  0x558f29c7e22c in box_process_rw+ec
 | #10 0x558f29c81743 in box_process1+8b
 | #11 0x558f29c81d5c in box_upsert+c4
 | #12 0x558f29caf110 in lbox_upsert+131
 | #13 0x558f29cfed97 in lj_BC_FUNCC+34
 | #14 0x558f29d104a4 in lua_pcall+34
 | #15 0x558f29cc7b09 in luaT_call+29
 | #16 0x558f29cc1de5 in lua_fiber_run_f+74
 | #17 0x558f29bd30d8 in _ZL16fiber_cxx_invokePFiP13__va_list_tagES0_+1e
 | #18 0x558f29cdca33 in fiber_loop+41
 | #19 0x558f29e4e8cd in coro_init+4c

To fix this issue, let's recall that when a DDL operation completes,
all pending transactions that affect the altered space are aborted by
the space_invalidate callback. So to avoid the crash, we just need to
bail out early from the on_replace trigger callback if we detect that
the current transaction has been aborted.

Closes #4152
---
https://github.com/tarantool/tarantool/issues/4152
https://github.com/tarantool/tarantool/commits/dv/vy-ddl-fixes

 src/box/vinyl.c                |  9 +++++
 src/box/vy_scheduler.c         |  5 +++
 src/lib/core/errinj.h          |  1 +
 test/box/errinj.result         | 10 +++--
 test/vinyl/errinj_ddl.result   | 90 ++++++++++++++++++++++++++++++++++++++++++
 test/vinyl/errinj_ddl.test.lua | 42 ++++++++++++++++++++
 6 files changed, 153 insertions(+), 4 deletions(-)

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 776643a8..9428e934 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -4070,6 +4070,15 @@ vy_build_on_replace(struct trigger *trigger, void *event)
 	}
 	return;
 err:
+	/*
+	 * No need to abort the DDL request if this transaction
+	 * has been aborted as its changes won't be applied to
+	 * the space anyway. Besides, it might have been aborted
+	 * by the DDL request itself, in which case the build
+	 * context isn't valid and so we must not modify it.
+	 */
+	if (tx->state == VINYL_TX_ABORT)
+		return;
 	ctx->is_failed = true;
 	diag_move(diag_get(), &ctx->diag);
 }
diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
index d56a505a..fabb4bb4 100644
--- a/src/box/vy_scheduler.c
+++ b/src/box/vy_scheduler.c
@@ -1091,6 +1091,11 @@ fail:
 static int
 vy_task_dump_execute(struct vy_task *task)
 {
+	struct errinj *errinj = errinj(ERRINJ_VY_DUMP_DELAY, ERRINJ_BOOL);
+	if (errinj != NULL && errinj->bparam) {
+		while (errinj->bparam)
+			fiber_sleep(0.01);
+	}
 	/*
 	 * Don't compress L1 runs as they are most frequently read
 	 * and smallest runs at the same time and so we would gain
diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
index 5d9317d5..4bca81ca 100644
--- a/src/lib/core/errinj.h
+++ b/src/lib/core/errinj.h
@@ -123,6 +123,7 @@ struct errinj {
 	_(ERRINJ_VY_INDEX_FILE_RENAME, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_RELAY_BREAK_LSN, ERRINJ_INT, {.iparam = -1}) \
 	_(ERRINJ_WAL_BREAK_LSN, ERRINJ_INT, {.iparam = -1}) \
+	_(ERRINJ_VY_DUMP_DELAY, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_VY_COMPACTION_DELAY, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_TUPLE_FORMAT_COUNT, ERRINJ_INT, {.iparam = -1}) \
 	_(ERRINJ_MEMTX_DELAY_GC, ERRINJ_BOOL, {.bparam = false}) \
diff --git a/test/box/errinj.result b/test/box/errinj.result
index 68b137a1..5905f8ee 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -40,22 +40,24 @@ errinj.info()
     state: false
   ERRINJ_VY_INDEX_FILE_RENAME:
     state: false
+  ERRINJ_VY_POINT_ITER_WAIT:
+    state: false
   ERRINJ_VY_DELAY_PK_LOOKUP:
     state: false
   ERRINJ_VY_TASK_COMPLETE:
     state: false
   ERRINJ_PORT_DUMP:
     state: false
-  ERRINJ_VY_POINT_ITER_WAIT:
-    state: false
+  ERRINJ_WAL_BREAK_LSN:
+    state: -1
   ERRINJ_WAL_IO:
     state: false
   ERRINJ_WAL_FALLOCATE:
     state: 0
-  ERRINJ_WAL_BREAK_LSN:
-    state: -1
   ERRINJ_LOG_ROTATE:
     state: false
+  ERRINJ_VY_DUMP_DELAY:
+    state: false
   ERRINJ_TUPLE_FORMAT_COUNT:
     state: -1
   ERRINJ_TUPLE_ALLOC:
diff --git a/test/vinyl/errinj_ddl.result b/test/vinyl/errinj_ddl.result
index 45e116a1..7bee4efd 100644
--- a/test/vinyl/errinj_ddl.result
+++ b/test/vinyl/errinj_ddl.result
@@ -804,6 +804,96 @@ s:select()
 s:drop()
 ---
 ...
+--
+-- gh-4152: yet another DML vs DDL race.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+---
+...
+_ = s:create_index('primary')
+---
+...
+s:replace{1, 1}
+---
+- [1, 1]
+...
+box.snapshot()
+---
+- ok
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+-- Create a secondary index. Delay dump.
+box.error.injection.set('ERRINJ_VY_DUMP_DELAY', true);
+---
+- ok
+...
+ch = fiber.channel(1);
+---
+...
+_ = fiber.create(function()
+    local status, err = pcall(s.create_index, s, 'secondary',
+                    {unique = true, parts = {2, 'unsigned'}})
+    ch:put(status or err)
+end);
+---
+...
+-- Wait for dump to start.
+test_run:wait_cond(function()
+    return box.stat.vinyl().scheduler.tasks_inprogress > 0
+end);
+---
+- true
+...
+-- Issue a DML request that will yield to check the unique
+-- constraint of the new index. Delay disk read.
+box.error.injection.set('ERRINJ_VY_READ_PAGE_DELAY', true);
+---
+- ok
+...
+_ = fiber.create(function()
+    local status, err = pcall(s.replace, s, {2, 1})
+    ch:put(status or err)
+end);
+---
+...
+-- Wait for index creation to complete.
+-- It must complete successfully.
+box.error.injection.set('ERRINJ_VY_DUMP_DELAY', false);
+---
+- ok
+...
+ch:get();
+---
+- true
+...
+-- Wait for the DML request to complete.
+-- It must be aborted.
+box.error.injection.set('ERRINJ_VY_READ_PAGE_DELAY', false);
+---
+- ok
+...
+ch:get();
+---
+- Transaction has been aborted by conflict
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+s.index.primary:select()
+---
+- - [1, 1]
+...
+s.index.secondary:select()
+---
+- - [1, 1]
+...
+s:drop()
+---
+...
 box.cfg{vinyl_cache = default_vinyl_cache}
 ---
 ...
diff --git a/test/vinyl/errinj_ddl.test.lua b/test/vinyl/errinj_ddl.test.lua
index 5870a0b0..52d9de65 100644
--- a/test/vinyl/errinj_ddl.test.lua
+++ b/test/vinyl/errinj_ddl.test.lua
@@ -353,4 +353,46 @@ ch4:get()
 s:select()
 s:drop()
 
+--
+-- gh-4152: yet another DML vs DDL race.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+_ = s:create_index('primary')
+s:replace{1, 1}
+box.snapshot()
+
+test_run:cmd("setopt delimiter ';'")
+-- Create a secondary index. Delay dump.
+box.error.injection.set('ERRINJ_VY_DUMP_DELAY', true);
+ch = fiber.channel(1);
+_ = fiber.create(function()
+    local status, err = pcall(s.create_index, s, 'secondary',
+                    {unique = true, parts = {2, 'unsigned'}})
+    ch:put(status or err)
+end);
+-- Wait for dump to start.
+test_run:wait_cond(function()
+    return box.stat.vinyl().scheduler.tasks_inprogress > 0
+end);
+-- Issue a DML request that will yield to check the unique
+-- constraint of the new index. Delay disk read.
+box.error.injection.set('ERRINJ_VY_READ_PAGE_DELAY', true);
+_ = fiber.create(function()
+    local status, err = pcall(s.replace, s, {2, 1})
+    ch:put(status or err)
+end);
+-- Wait for index creation to complete.
+-- It must complete successfully.
+box.error.injection.set('ERRINJ_VY_DUMP_DELAY', false);
+ch:get();
+-- Wait for the DML request to complete.
+-- It must be aborted.
+box.error.injection.set('ERRINJ_VY_READ_PAGE_DELAY', false);
+ch:get();
+test_run:cmd("setopt delimiter ''");
+
+s.index.primary:select()
+s.index.secondary:select()
+s:drop()
+
 box.cfg{vinyl_cache = default_vinyl_cache}
-- 
2.11.0

[PATCH 2/2] vinyl: fix crash if space is dropped while space.get is reading from it

[Vladimir Davydov]

In contrast to vinyl_iterator, vinyl_index_get doesn't take a reference
to the LSM tree while reading from it. As a result, if the LSM tree is
dropped in the meantime, vinyl_index_get will crash. Fix this issue by
surrounding vy_get with vy_lsm_ref/unref.

Closes #4109
---
https://github.com/tarantool/tarantool/issues/4109
https://github.com/tarantool/tarantool/commits/dv/vy-ddl-fixes

 src/box/vinyl.c                |  9 +++++++-
 test/vinyl/errinj_ddl.result   | 49 ++++++++++++++++++++++++++++++++++++++++++
 test/vinyl/errinj_ddl.test.lua | 20 +++++++++++++++++
 3 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 9428e934..41918beb 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -3984,7 +3984,14 @@ vinyl_index_get(struct index *index, const char *key,
 		return -1;
 	}
 
-	if (vy_get_by_raw_key(lsm, tx, rv, key, part_count, ret) != 0)
+	/*
+	 * Make sure the LSM tree isn't deleted while we are
+	 * reading from it.
+	 */
+	vy_lsm_ref(lsm);
+	int rc = vy_get_by_raw_key(lsm, tx, rv, key, part_count, ret);
+	vy_lsm_unref(lsm);
+	if (rc != 0)
 		return -1;
 	if (*ret != NULL) {
 		tuple_bless(*ret);
diff --git a/test/vinyl/errinj_ddl.result b/test/vinyl/errinj_ddl.result
index 7bee4efd..dcbf90f7 100644
--- a/test/vinyl/errinj_ddl.result
+++ b/test/vinyl/errinj_ddl.result
@@ -894,6 +894,55 @@ s.index.secondary:select()
 s:drop()
 ---
 ...
+--
+-- gh-4109: crash if space is dropped while space.get is reading from it.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+---
+...
+_ = s:create_index('primary')
+---
+...
+s:replace{1}
+---
+- [1]
+...
+box.snapshot()
+---
+- ok
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+box.error.injection.set('ERRINJ_VY_READ_PAGE_TIMEOUT', 0.01);
+---
+- ok
+...
+ch = fiber.channel(1);
+---
+...
+_ = fiber.create(function()
+    local _, ret = pcall(s.get, s, 1)
+    ch:put(ret)
+end);
+---
+...
+s:drop();
+---
+...
+ch:get();
+---
+- [1]
+...
+box.error.injection.set('ERRINJ_VY_READ_PAGE_TIMEOUT', 0);
+---
+- ok
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
 box.cfg{vinyl_cache = default_vinyl_cache}
 ---
 ...
diff --git a/test/vinyl/errinj_ddl.test.lua b/test/vinyl/errinj_ddl.test.lua
index 52d9de65..6413c352 100644
--- a/test/vinyl/errinj_ddl.test.lua
+++ b/test/vinyl/errinj_ddl.test.lua
@@ -395,4 +395,24 @@ s.index.primary:select()
 s.index.secondary:select()
 s:drop()
 
+--
+-- gh-4109: crash if space is dropped while space.get is reading from it.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+_ = s:create_index('primary')
+s:replace{1}
+box.snapshot()
+
+test_run:cmd("setopt delimiter ';'")
+box.error.injection.set('ERRINJ_VY_READ_PAGE_TIMEOUT', 0.01);
+ch = fiber.channel(1);
+_ = fiber.create(function()
+    local _, ret = pcall(s.get, s, 1)
+    ch:put(ret)
+end);
+s:drop();
+ch:get();
+box.error.injection.set('ERRINJ_VY_READ_PAGE_TIMEOUT', 0);
+test_run:cmd("setopt delimiter ''");
+
 box.cfg{vinyl_cache = default_vinyl_cache}
-- 
2.11.0

Re: [PATCH 2/2] vinyl: fix crash if space is dropped while space.get is reading from it

[Vladimir Davydov]

Both fixes are straightforward. Pushed to master, 2.1, and 1.10.