From afe090767212fed32cef1203390a56ae6436e390 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov@tarantool.org>
Date: Wed, 7 Aug 2024 13:18:54 +0300
Subject: [PATCH] vinyl: do not abort unrelated transactions on DDL
Since commit 8f4be3227635 ("txm: disallow yields after DDL operation in
TX"), any DDL operation aborts **all** active transactions, even those
that wouldn't be affected by it anyway, see `memtx_engine_prepare()`,
`memtx_tx_abort_all_for_ddl()`. Actually, there's no need to do that in
Vinyl because it properly handles concurrent DDL operations, see commit
d3e123695651 ("vinyl: abort affected transactions when space is removed
from cache"). Let's skip Vinyl transactions from consideration by
marking the Vinyl engine with a special flag.
Closes #10375
NO_DOC=bug fix
(cherry picked from commit f5f061d051dc6268949bfcb141d211142282578d)
---
...375-vy-do-not-abort-unrelated-tx-on-ddl.md | 3 +
src/box/engine.h | 6 ++
src/box/memtx_tx.c | 2 +
src/box/txn.c | 10 ++-
src/box/txn.h | 6 ++
src/box/vinyl.c | 1 +
..._not_abort_unrelated_transactions_test.lua | 65 +++++++++++++++++++
7 files changed, 92 insertions(+), 1 deletion(-)
create mode 100644 changelogs/unreleased/gh-10375-vy-do-not-abort-unrelated-tx-on-ddl.md
create mode 100644 test/engine-luatest/gh_10375_ddl_does_not_abort_unrelated_transactions_test.lua
diff --git a/changelogs/unreleased/gh-10375-vy-do-not-abort-unrelated-tx-on-ddl.md b/changelogs/unreleased/gh-10375-vy-do-not-abort-unrelated-tx-on-ddl.md
new file mode 100644
index 0000000000..d18048e147
--- /dev/null
+++ b/changelogs/unreleased/gh-10375-vy-do-not-abort-unrelated-tx-on-ddl.md
@@ -0,0 +1,3 @@
+## bugfix/vinyl
+
+* Fixed a bug when any DDL operation aborted unrelated transactions (gh-10375).
diff --git a/src/box/engine.h b/src/box/engine.h
index bd7d102526..85febfb204 100644
--- a/src/box/engine.h
+++ b/src/box/engine.h
@@ -263,6 +263,12 @@ enum {
* Set if the engine supports creation of a read view.
*/
ENGINE_SUPPORTS_READ_VIEW = 1 << 1,
+ /**
+ * Set if the engine's transaction manager properly handles
+ * concurrent DDL operations. A DDL operation will abort all
+ * transactions for engines that don't have this flag set.
+ */
+ ENGINE_TXM_HANDLES_DDL = 1 << 5,
};
struct engine {
diff --git a/src/box/memtx_tx.c b/src/box/memtx_tx.c
index f0886880f7..3082147aee 100644
--- a/src/box/memtx_tx.c
+++ b/src/box/memtx_tx.c
@@ -689,6 +689,8 @@ memtx_tx_abort_all_for_ddl(struct txn *ddl_owner)
rlist_foreach_entry(to_be_aborted, &txm.all_txs, in_all_txs) {
if (to_be_aborted == ddl_owner)
continue;
+ if (txn_has_flag(to_be_aborted, TXN_HANDLES_DDL))
+ continue;
if (to_be_aborted->status != TXN_INPROGRESS &&
to_be_aborted->status != TXN_IN_READ_VIEW)
continue;
diff --git a/src/box/txn.c b/src/box/txn.c
index a85ab111ea..ec42ce5f92 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -509,6 +509,11 @@ txn_begin(void)
* if they are not supported.
*/
txn_set_flags(txn, TXN_CAN_YIELD);
+ /*
+ * A transaction is unaffected by concurrent DDL as long as it has
+ * no statements.
+ */
+ txn_set_flags(txn, TXN_HANDLES_DDL);
memtx_tx_register_txn(txn);
rmean_collect(rmean_box, IPROTO_BEGIN, 1);
return txn;
@@ -521,7 +526,10 @@ txn_begin_in_engine(struct engine *engine, struct txn *txn)
return 0;
if (txn->engine == NULL) {
txn->engine = engine;
- return engine_begin(engine, txn);
+ if (engine_begin(engine, txn) != 0)
+ return -1;
+ if ((engine->flags & ENGINE_TXM_HANDLES_DDL) == 0)
+ txn_clear_flags(txn, TXN_HANDLES_DDL);
} else if (txn->engine != engine) {
/**
* Only one engine can be used in
diff --git a/src/box/txn.h b/src/box/txn.h
index 526453aa2e..e946f66cf5 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -106,6 +106,12 @@ enum txn_flag {
* rolled back at commit.
*/
TXN_IS_ABORTED_BY_TIMEOUT = 0x100,
+ /**
+ * Transaction properly handles concurrent DDL operations.
+ * If a transaction doesn't have this flag, it'll be aborted
+ * by any DDL operation.
+ */
+ TXN_HANDLES_DDL = 0x1000,
};
enum {
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 067e565930..fe56d59630 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -2739,6 +2739,7 @@ vinyl_engine_new(const char *dir, size_t memory,
env->base.vtab = &vinyl_engine_vtab;
env->base.name = "vinyl";
+ env->base.flags = ENGINE_TXM_HANDLES_DDL;
return &env->base;
}
diff --git a/test/engine-luatest/gh_10375_ddl_does_not_abort_unrelated_transactions_test.lua b/test/engine-luatest/gh_10375_ddl_does_not_abort_unrelated_transactions_test.lua
new file mode 100644
index 0000000000..329a13184d
--- /dev/null
+++ b/test/engine-luatest/gh_10375_ddl_does_not_abort_unrelated_transactions_test.lua
@@ -0,0 +1,65 @@
+local t = require('luatest')
+local server = require('luatest.server')
+
+local g = t.group(nil, t.helpers.matrix{engine = {'memtx', 'vinyl'}})
+
+g.before_all(function(cg)
+ cg.server = server:new({
+ box_cfg = {memtx_use_mvcc_engine = true},
+ })
+ 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.test1 ~= nil then
+ box.space.test1:drop()
+ end
+ if box.space.test2 ~= nil then
+ box.space.test2:drop()
+ end
+ end)
+end)
+
+g.test_ddl_does_not_abort_unrelated_transactions = function(cg)
+ t.skip_if(cg.params.engine == 'memtx', 'gh-10377')
+ cg.server:exec(function(engine)
+ local fiber = require('fiber')
+ box.schema.create_space('test1', {engine = engine})
+ box.space.test1:create_index('primary')
+ box.begin()
+ box.space.test1:insert({1, 10})
+ local f = fiber.new(function()
+ box.schema.create_space('test2', {engine = engine})
+ box.space.test2:create_index('primary')
+ end)
+ f:set_joinable(true)
+ t.assert_equals({f:join()}, {true})
+ t.assert_equals({pcall(box.commit)}, {true})
+ t.assert_equals(box.space.test1:select(), {{1, 10}})
+ end, {cg.params.engine})
+end
+
+g.test_ddl_aborts_related_transactions = function(cg)
+ cg.server:exec(function(engine)
+ local fiber = require('fiber')
+ box.schema.create_space('test1', {engine = engine})
+ box.space.test1:create_index('primary')
+ box.begin()
+ box.space.test1:insert({1, 10})
+ local f = fiber.new(function()
+ box.space.test1:create_index('secondary', {parts = {2, 'unsigned'}})
+ end)
+ f:set_joinable(true)
+ t.assert_equals({f:join()}, {true})
+ t.assert_error_covers({
+ type = 'ClientError',
+ code = box.error.TRANSACTION_CONFLICT,
+ }, box.commit)
+ t.assert_equals(box.space.test1:select(), {})
+ end, {cg.params.engine})
+end
--
GitLab