From 05fa2f74fb9604bd0d4beb7e86d3145261a7188b Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov@tarantool.org>
Date: Thu, 6 Jun 2024 11:27:13 +0300
Subject: [PATCH] vinyl: fix crash on extending secondary key parts with
primary
If a secondary index is altered in such a way that its key parts are
extended with the primary key parts, rebuild isn't required because
`cmp_def` doesn't change, see `vinyl_index_def_change_requires_rebuild`.
In this case `vinyl_index_update_def` will try to update `key_def` and
`cmp_def` in-place with `key_def_copy`. This will lead to a crash
because the number of parts in the new `key_def` is greater.
We can't use `key_def_dup` instead of `key_def_copy` there because
there may be read iterators using the old `key_def` by pointer so
there's no other option but to force rebuild in this case.
The bug was introduced in commit 64817066ff60 ("vinyl: use update_def
index method to update vy_lsm on ddl").
Closes #10095
NO_DOC=bug fix
(cherry picked from commit 9b817848c4e87665aa38133f97b86ff6148279a1)
---
.../gh-10095-vy-index-update-def-fix.md | 4 +++
src/box/vinyl.c | 16 ++++++++-
.../gh_10095_update_index_def_test.lua | 35 +++++++++++++++++++
3 files changed, 54 insertions(+), 1 deletion(-)
create mode 100644 changelogs/unreleased/gh-10095-vy-index-update-def-fix.md
create mode 100644 test/vinyl-luatest/gh_10095_update_index_def_test.lua
diff --git a/changelogs/unreleased/gh-10095-vy-index-update-def-fix.md b/changelogs/unreleased/gh-10095-vy-index-update-def-fix.md
new file mode 100644
index 0000000000..7a40071c99
--- /dev/null
+++ b/changelogs/unreleased/gh-10095-vy-index-update-def-fix.md
@@ -0,0 +1,4 @@
+## bugfix/vinyl
+
+* Fixed a bug when a DDL operation crashed in case of extending the key parts
+ of a secondary index with the primary index key parts (gh-10095).
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index cd76d065aa..5ac3d384d3 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -929,6 +929,11 @@ vinyl_index_update_def(struct index *index)
{
struct vy_lsm *lsm = vy_lsm(index);
lsm->opts = index->def->opts;
+ /*
+ * Sic: We copy key definitions in-place instead of reallocating them
+ * because they may be used by read iterators by pointer, for example,
+ * see vy_run_iterator.
+ */
key_def_copy(lsm->key_def, index->def->key_def);
key_def_copy(lsm->cmp_def, index->def->cmp_def);
}
@@ -960,7 +965,9 @@ vinyl_index_def_change_requires_rebuild(struct index *index,
return true;
assert(index_depends_on_pk(index));
+ const struct key_def *old_key_def = old_def->key_def;
const struct key_def *old_cmp_def = old_def->cmp_def;
+ const struct key_def *new_key_def = new_def->key_def;
const struct key_def *new_cmp_def = new_def->cmp_def;
/*
@@ -970,8 +977,15 @@ vinyl_index_def_change_requires_rebuild(struct index *index,
* won't reveal such statements, but we may still need to
* compare them to statements inserted after ALTER hence
* we can't narrow field types without index rebuild.
+ *
+ * Sic: If secondary index key parts are extended with primary
+ * index key parts, cmp_def (hence the sorting order) will stay
+ * the same, but we still have to rebuild the index because
+ * the new key_def has more parts so we can't update it in-place,
+ * see vinyl_index_update_def().
*/
- if (old_cmp_def->part_count != new_cmp_def->part_count)
+ if (old_key_def->part_count != new_key_def->part_count ||
+ old_cmp_def->part_count != new_cmp_def->part_count)
return true;
for (uint32_t i = 0; i < new_cmp_def->part_count; i++) {
diff --git a/test/vinyl-luatest/gh_10095_update_index_def_test.lua b/test/vinyl-luatest/gh_10095_update_index_def_test.lua
new file mode 100644
index 0000000000..45c93483ae
--- /dev/null
+++ b/test/vinyl-luatest/gh_10095_update_index_def_test.lua
@@ -0,0 +1,35 @@
+local server = require('luatest.server')
+local t = require('luatest')
+
+local g = t.group()
+
+g.before_all(function(cg)
+ 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()
+ if box.space.test ~= nil then
+ box.space.test:drop()
+ end
+ end)
+end)
+
+g.test_update_index_def = function(cg)
+ cg.server:exec(function()
+ local s = box.schema.create_space('test', {engine = 'vinyl'})
+ s:create_index('pk')
+ s:create_index('sk', {parts = {{2, 'unsigned'}}})
+ s:insert({1, 10, 100})
+ t.assert_equals(s.index.sk:get({10}), {1, 10, 100})
+ s.index.sk:alter({parts = {{2, 'unsigned'}, {1, 'unsigned'}}})
+ t.assert_equals(s.index.sk:get({10, 1}), {1, 10, 100})
+ s.index.sk:alter({parts = {{2, 'unsigned'}, {3, 'unsigned'}}})
+ t.assert_equals(s.index.sk:get({10, 100}), {1, 10, 100})
+ end)
+end
--
GitLab