From 1babcf1ea49d9c8823a706a36d1a12ef43733d28 Mon Sep 17 00:00:00 2001
From: Ilya Verbin <iverbin@tarantool.org>
Date: Tue, 10 Oct 2023 19:12:53 +0300
Subject: [PATCH] box: fix space:bsize() handling on space alter
During building an index in background, some transaction can perform a dml
request that affects space size (e.g. a replace), but the size will remain
the same, because bsize is moved from the old space to the new space in
memtx_space_prepare_alter() prior to space_execute_dml(). Fix this issue by
calling space_finish_alter() in alter_space_do().
In fact, this patch partially reverts commit 9ec3b1a445a6 ("alter: zap
space_vtab::commit_alter").
NO_DOC=bugfix
Closes #9247
(cherry picked from commit 54a42186de2776b4fa970ee1a9cf53c543a1cb26)
---
...fix-space-bsize-handling-on-space-alter.md | 4 +
src/box/alter.cc | 1 +
src/box/blackhole.c | 1 +
src/box/memtx_space.c | 20 ++-
src/box/session_settings.c | 1 +
src/box/space.c | 7 ++
src/box/space.h | 14 +++
src/box/sysview.c | 1 +
src/box/vinyl.c | 1 +
...bsize_on_concurrent_dml_and_alter_test.lua | 118 ++++++++++++++++++
10 files changed, 166 insertions(+), 2 deletions(-)
create mode 100644 changelogs/unreleased/gh-9247-fix-space-bsize-handling-on-space-alter.md
create mode 100644 test/box-luatest/gh_9247_bsize_on_concurrent_dml_and_alter_test.lua
diff --git a/changelogs/unreleased/gh-9247-fix-space-bsize-handling-on-space-alter.md b/changelogs/unreleased/gh-9247-fix-space-bsize-handling-on-space-alter.md
new file mode 100644
index 0000000000..e11362fca6
--- /dev/null
+++ b/changelogs/unreleased/gh-9247-fix-space-bsize-handling-on-space-alter.md
@@ -0,0 +1,4 @@
+## bugfix/core
+
+* Fixed a bug that could result in the incorrect `space:bsize()` when altering
+ a primary index concurrently with DML operations (gh-9247).
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 51abc28fe7..8d8d40a2b8 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1017,6 +1017,7 @@ alter_space_do(struct txn_stmt *stmt, struct alter_space *alter)
* The new space is ready. Time to update the space
* cache with it.
*/
+ space_finish_alter(alter->old_space, alter->new_space);
space_cache_replace(alter->old_space, alter->new_space);
space_detach_constraints(alter->old_space);
space_unpin_collations(alter->old_space);
diff --git a/src/box/blackhole.c b/src/box/blackhole.c
index 63b102aae3..83e1d8bb96 100644
--- a/src/box/blackhole.c
+++ b/src/box/blackhole.c
@@ -126,6 +126,7 @@ static const struct space_vtab blackhole_space_vtab = {
/* .build_index = */ generic_space_build_index,
/* .swap_index = */ generic_space_swap_index,
/* .prepare_alter = */ generic_space_prepare_alter,
+ /* .finish_alter = */ generic_space_finish_alter,
/* .prepare_upgrade = */ generic_space_prepare_upgrade,
/* .invalidate = */ generic_space_invalidate,
};
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index e36d5bf6e6..3f6594bd65 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -1052,7 +1052,6 @@ memtx_space_drop_primary_key(struct space *space)
* can be put back online properly.
*/
memtx_space->replace = memtx_space_replace_no_keys;
- memtx_space->bsize = 0;
}
static void
@@ -1369,10 +1368,26 @@ memtx_space_prepare_alter(struct space *old_space, struct space *new_space)
}
new_memtx_space->replace = old_memtx_space->replace;
- new_memtx_space->bsize = old_memtx_space->bsize;
return 0;
}
+/**
+ * Copy bsize to the newly altered space from the old space.
+ * In case of DropIndex or TruncateIndex alter operations, the new space will be
+ * empty, and bsize must not be copied.
+ */
+static void
+memtx_space_finish_alter(struct space *old_space, struct space *new_space)
+{
+ struct memtx_space *old_memtx_space = (struct memtx_space *)old_space;
+ struct memtx_space *new_memtx_space = (struct memtx_space *)new_space;
+
+ bool is_empty = new_space->index_count == 0 ||
+ index_size(new_space->index[0]) == 0;
+ if (!is_empty)
+ new_memtx_space->bsize = old_memtx_space->bsize;
+}
+
/* }}} DDL */
static const struct space_vtab memtx_space_vtab = {
@@ -1395,6 +1410,7 @@ static const struct space_vtab memtx_space_vtab = {
/* .build_index = */ memtx_space_build_index,
/* .swap_index = */ generic_space_swap_index,
/* .prepare_alter = */ memtx_space_prepare_alter,
+ /* .finish_alter = */ memtx_space_finish_alter,
/* .prepare_upgrade = */ memtx_space_prepare_upgrade,
/* .invalidate = */ generic_space_invalidate,
};
diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index 66fa9fc830..9c38cd3f96 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -437,6 +437,7 @@ const struct space_vtab session_settings_space_vtab = {
/* .build_index = */ generic_space_build_index,
/* .swap_index = */ generic_space_swap_index,
/* .prepare_alter = */ generic_space_prepare_alter,
+ /* .finish_alter = */ generic_space_finish_alter,
/* .prepare_upgrade = */ generic_space_prepare_upgrade,
/* .invalidate = */ generic_space_invalidate,
};
diff --git a/src/box/space.c b/src/box/space.c
index b7fbf6fdbc..4a2a51b7dd 100644
--- a/src/box/space.c
+++ b/src/box/space.c
@@ -971,6 +971,13 @@ generic_space_prepare_alter(struct space *old_space, struct space *new_space)
return 0;
}
+void
+generic_space_finish_alter(struct space *old_space, struct space *new_space)
+{
+ (void)old_space;
+ (void)new_space;
+}
+
int
generic_space_prepare_upgrade(struct space *old_space, struct space *new_space)
{
diff --git a/src/box/space.h b/src/box/space.h
index 9a101caf0e..930184cf32 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -144,6 +144,12 @@ struct space_vtab {
*/
int (*prepare_alter)(struct space *old_space,
struct space *new_space);
+ /**
+ * Notify the engine after altering a space and before replacing
+ * old_space with new_space in the space cache.
+ */
+ void (*finish_alter)(struct space *old_space,
+ struct space *new_space);
/** Prepares a space for online upgrade on alter. */
int (*prepare_upgrade)(struct space *old_space,
struct space *new_space);
@@ -563,6 +569,13 @@ space_prepare_alter(struct space *old_space, struct space *new_space)
return new_space->vtab->prepare_alter(old_space, new_space);
}
+static inline void
+space_finish_alter(struct space *old_space, struct space *new_space)
+{
+ assert(old_space->vtab == new_space->vtab);
+ new_space->vtab->finish_alter(old_space, new_space);
+}
+
static inline int
space_prepare_upgrade(struct space *old_space, struct space *new_space)
{
@@ -668,6 +681,7 @@ int generic_space_check_format(struct space *, struct tuple_format *);
int generic_space_build_index(struct space *, struct index *,
struct tuple_format *, bool);
int generic_space_prepare_alter(struct space *, struct space *);
+void generic_space_finish_alter(struct space *, struct space *);
int generic_space_prepare_upgrade(struct space *old_space,
struct space *new_space);
void generic_space_invalidate(struct space *);
diff --git a/src/box/sysview.c b/src/box/sysview.c
index 6b3dfd790d..8b655b291d 100644
--- a/src/box/sysview.c
+++ b/src/box/sysview.c
@@ -505,6 +505,7 @@ static const struct space_vtab sysview_space_vtab = {
/* .build_index = */ generic_space_build_index,
/* .swap_index = */ generic_space_swap_index,
/* .prepare_alter = */ generic_space_prepare_alter,
+ /* .finish_alter = */ generic_space_finish_alter,
/* .prepare_upgrade = */ generic_space_prepare_upgrade,
/* .invalidate = */ generic_space_invalidate,
};
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 2cb41a222d..259c1b9f68 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -4641,6 +4641,7 @@ static const struct space_vtab vinyl_space_vtab = {
/* .build_index = */ vinyl_space_build_index,
/* .swap_index = */ vinyl_space_swap_index,
/* .prepare_alter = */ vinyl_space_prepare_alter,
+ /* .finish_alter = */ generic_space_finish_alter,
/* .prepare_upgrade = */ generic_space_prepare_upgrade,
/* .invalidate = */ vinyl_space_invalidate,
};
diff --git a/test/box-luatest/gh_9247_bsize_on_concurrent_dml_and_alter_test.lua b/test/box-luatest/gh_9247_bsize_on_concurrent_dml_and_alter_test.lua
new file mode 100644
index 0000000000..5fe9a9bdf6
--- /dev/null
+++ b/test/box-luatest/gh_9247_bsize_on_concurrent_dml_and_alter_test.lua
@@ -0,0 +1,118 @@
+local server = require('luatest.server')
+local t = require('luatest')
+local g = t.group('gh-9247')
+
+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()
+ box.space.test:drop()
+ end)
+end)
+
+-- Based on `test/box/alter-primary-index-tuple-leak-long.test.lua'.
+local function concurrent_insert_and_alter_helper(cg, rollback)
+ cg.server:exec(function(rollback)
+ local fiber = require('fiber')
+ local s = box.schema.space.create('test')
+ local idx = s:create_index('pk')
+ local alter_started = false
+ local data = string.rep('a', 66)
+
+ -- Create a table with enough tuples to make primary index alter in
+ -- background.
+ for i = 0, 7000, 100 do
+ box.begin()
+ for j = i, i + 99 do
+ s:insert{j, j, data}
+ end
+ box.commit()
+ end
+
+ -- Wait for fiber yield during altering and insert some tuples.
+ local fib_insert = fiber.new(function()
+ while not alter_started do
+ fiber.sleep(0)
+ end
+ box.begin()
+ for i = 8000, 9000 do
+ s:insert{i, i, data}
+ end
+ box.commit()
+ end)
+
+ -- Alter primary index.
+ -- If the index will not be altered in background, the test will fail
+ -- after timeout (fib_insert:join() will wait forever).
+ local fib_alter = fiber.new(function()
+ alter_started = true
+ box.begin()
+ idx:alter{parts = {{field = 2}}}
+ if rollback then
+ box.rollback()
+ else
+ box.commit()
+ end
+ alter_started = false
+ end)
+
+ fib_insert:set_joinable(true)
+ fib_alter:set_joinable(true)
+ fib_insert:join()
+ fib_alter:join()
+
+ local bsize = 0
+ for _, tuple in s:pairs() do
+ bsize = bsize + tuple:bsize()
+ end
+ t.assert_equals(s:bsize(), bsize)
+ end, {rollback})
+end
+
+-- Check that space:bsize() is correct for a built-in-background primary index
+-- with concurrent inserts.
+g.test_concurrent_insert_and_alter_commit = function(cg)
+ concurrent_insert_and_alter_helper(cg, false)
+end
+
+-- Check that space:bsize() is correct for a built-in-background primary index
+-- with concurrent inserts.
+g.test_concurrent_insert_and_alter_rollback = function(cg)
+ concurrent_insert_and_alter_helper(cg, true)
+end
+
+local function truncate_helper(cg, rollback)
+ cg.server:exec(function(rollback)
+ local s = box.schema.space.create('test')
+ s:create_index('pk')
+ s:insert{1, 2, 3}
+ local bsize_before_truncate = s:bsize()
+
+ box.begin()
+ s:truncate()
+ if rollback then
+ box.rollback()
+ t.assert_equals(s:bsize(), bsize_before_truncate)
+ else
+ box.commit()
+ t.assert_equals(s:bsize(), 0)
+ end
+ end, {rollback})
+end
+
+-- Check that space:bsize() is correct after successful space:truncate().
+g.test_truncate_commit = function(cg)
+ truncate_helper(cg, false)
+end
+
+-- Check that space:bsize() is correct after rolled back space:truncate().
+g.test_truncate_rollback = function(cg)
+ truncate_helper(cg, true)
+end
--
GitLab