commit 5e3ed27ca3a9a5ec9b82211b7c3e1131b6446082 from: Vladimir Davydov date: Thu Aug 08 08:44:37 2024 UTC 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) commit - c04b9bfe704099074602c4d34f0f79462c99c4ee commit + 5e3ed27ca3a9a5ec9b82211b7c3e1131b6446082 blob - /dev/null blob + d18048e147f26955df8b65d7ef49d84dd3f61d8f (mode 644) --- /dev/null +++ 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). blob - 870f4d4cef195bcfdfb8bc5ab070e5e8d428bf73 blob + a5955a97946352c26b4534caaa8c06c437bae70c --- src/box/engine.h +++ src/box/engine.h @@ -278,6 +278,12 @@ enum { * Engine setting this flag must support read views. */ ENGINE_JOIN_BY_MEMTX = 1 << 3, + /** + * 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 { blob - 0f44c21123332186f3b7df39f872d554c43adcfd blob + e3bce440c5287c1432d1df2c13dc517d8b2e064f --- src/box/memtx_tx.c +++ src/box/memtx_tx.c @@ -689,6 +689,8 @@ memtx_tx_abort_all_for_ddl(struct txn *ddl_owner) struct txn *to_be_aborted; 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) blob - 71b8731a9763085d4f982303e42dc16cd097401c blob + 8f839bbc9cd834900108c5983e185b07e7c9e51b --- src/box/txn.c +++ src/box/txn.c @@ -543,6 +543,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; @@ -555,7 +560,10 @@ txn_begin_in_engine(struct engine *engine, struct 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 blob - 238f9aab344a93f65cd870ed65a42f48efd4ff87 blob + de3a3655a4db7674d5df4933aff676c060399cfa --- src/box/txn.h +++ src/box/txn.h @@ -110,6 +110,12 @@ enum txn_flag { * Transaction has been rolled back so it cannot be continued. */ TXN_IS_ROLLED_BACK = 0x200, + /** + * 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 { blob - 054b42321adc08b4528ae2f402c7010c650d7c49 blob + b8f7ad5ff2a3b578707c27c8a6c0c00a88b58545 --- src/box/vinyl.c +++ src/box/vinyl.c @@ -2744,6 +2744,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; } blob - /dev/null blob + 329a13184d191a3a89c015cce14880115200f7a0 (mode 644) --- /dev/null +++ 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