From b4304df7f16dc751dbd5ffe8acdb4ec51fa41584 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov@tarantool.org>
Date: Tue, 24 Sep 2024 15:20:29 +0300
Subject: [PATCH] vinyl: fix crash when empty PK DDL races with DML
Vinyl doesn't support altering the primary index of a non-empty space,
but the check forbidding this isn't entirely reliable - the DDL function
may yield to wait for pending WAL writes to finish after ensuring that
the space doesn't contain any tuples. If a new tuples is inserted into
the space in the meantime, the DDL operation will proceed rebuilding
the primary index and trigger a crash because the code is written on
the assumption that it's rebuilding a secondary index:
```
./src/box/vinyl.c:1572: vy_check_is_unique_secondary_one: Assertion `lsm->index_id > 0' failed.
```
Let's fix this by moving the check after syncing on WAL.
Closes #10603
NO_DOC=bug fix
(cherry picked from commit 955537b57c2aade58b7ca42501a9bbe50dd91f26)
---
.../gh-10603-fix-vinyl-pk-alter-crash.md | 4 +
src/box/vinyl.c | 12 +--
.../gh_10603_pk_alter_vs_dml_test.lua | 92 +++++++++++++++++++
3 files changed, 102 insertions(+), 6 deletions(-)
create mode 100644 changelogs/unreleased/gh-10603-fix-vinyl-pk-alter-crash.md
create mode 100644 test/vinyl-luatest/gh_10603_pk_alter_vs_dml_test.lua
diff --git a/changelogs/unreleased/gh-10603-fix-vinyl-pk-alter-crash.md b/changelogs/unreleased/gh-10603-fix-vinyl-pk-alter-crash.md
new file mode 100644
index 0000000000..a5fc88e8d8
--- /dev/null
+++ b/changelogs/unreleased/gh-10603-fix-vinyl-pk-alter-crash.md
@@ -0,0 +1,4 @@
+## bugfix/vinyl
+
+* Fixed a bug when an attempt to alter the primary index of an empty space
+ triggered a crash if executed concurrently with a DML request (gh-10603).
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 27e6708a89..ebe59d4c0a 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -4294,12 +4294,6 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
struct vy_lsm *pk = vy_lsm(src_space->index[0]);
struct txn *txn = in_txn();
- if (new_index->def->iid == 0 && !vy_lsm_is_empty(pk)) {
- diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
- "rebuilding the primary index of a non-empty space");
- return -1;
- }
-
struct errinj *inj = errinj(ERRINJ_BUILD_INDEX, ERRINJ_INT);
if (inj != NULL && inj->iparam == (int)new_index->def->iid) {
diag_set(ClientError, ER_INJECTION, "build index");
@@ -4327,6 +4321,12 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
if (!need_wal_sync && vy_lsm_is_empty(pk))
return 0; /* space is empty, nothing to do */
+ if (new_index->def->iid == 0) {
+ diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
+ "rebuilding the primary index of a non-empty space");
+ return -1;
+ }
+
if (txn_check_singlestatement(txn, "index build") != 0)
return -1;
diff --git a/test/vinyl-luatest/gh_10603_pk_alter_vs_dml_test.lua b/test/vinyl-luatest/gh_10603_pk_alter_vs_dml_test.lua
new file mode 100644
index 0000000000..a6fdf28d88
--- /dev/null
+++ b/test/vinyl-luatest/gh_10603_pk_alter_vs_dml_test.lua
@@ -0,0 +1,92 @@
+local server = require('luatest.server')
+local t = require('luatest')
+
+local g = t.group()
+
+g.before_all(function(cg)
+ t.tarantool.skip_if_not_debug()
+ cg.server = server:new()
+ cg.server:start()
+end)
+
+g.after_all(function(cg)
+ cg.server:drop()
+end)
+
+g.after_each(function(cg)
+ cg.server:exec(function()
+ box.error.injection.set('ERRINJ_WAL_DELAY', false)
+ if box.space.test ~= nil then
+ box.space.test:drop()
+ end
+ end)
+end)
+
+g.test_empty_pk_alter = function(cg)
+ cg.server:exec(function()
+ local fiber = require('fiber')
+ local timeout = 10 -- seconds
+
+ local s = box.schema.space.create('test', {engine = 'vinyl'})
+ s:create_index('pk')
+
+ local c1 = fiber.channel(1)
+ local f1 = fiber.new(function()
+ c1:get()
+ s:replace({1, 10})
+ end)
+ f1:set_joinable(true)
+
+ local c2 = fiber.channel(1)
+ local f2 = fiber.new(function()
+ c2:get()
+ s.index.pk:alter({parts = {2, 'unsigned'}})
+ end)
+ f2:set_joinable(true)
+
+ local c3 = fiber.channel(1)
+ local f3 = fiber.new(function()
+ c3:get()
+ s:replace({2, 20})
+ end)
+ f3:set_joinable(true)
+
+ -- Fiber 1 (DML) inserts a tuple into the space and blocks on WAL.
+ box.error.injection.set('ERRINJ_WAL_DELAY', true)
+ c1:put(true)
+ fiber.yield()
+
+ -- Fiber 2 (DDL) waits for pending WAL writes to complete.
+ c2:put(true)
+ fiber.yield()
+
+ -- Fiber 3 (DML) inserts a tuple into the space and blocks on WAL.
+ c3:put(true)
+ fiber.yield()
+
+ box.error.injection.set('ERRINJ_WAL_DELAY', false)
+ fiber.yield()
+
+ -- Fiber 1 (DML) completes successfully.
+ t.assert_equals({f1:join(timeout)}, {true})
+
+ -- Fiber 2 (DDL) fails because the primary index isn't empty.
+ t.assert_error_covers({
+ type = 'ClientError',
+ code = box.error.UNUSUPPORTED,
+ message = 'Vinyl does not support ' ..
+ 'rebuilding the primary index of a non-empty space',
+ }, function()
+ local ok, err = f2:join(timeout)
+ if not ok then
+ error(err)
+ end
+ end)
+
+ -- Fiber 3 (DML) completes successfully.
+ t.assert_equals({f3:join(timeout)}, {true})
+
+ t.assert_equals(box.space.test:select({}, {fullscan = true}),
+ {{1, 10}, {2, 20}})
+ end)
+end
--
GitLab