commit 6e1e1d50a703a26b4dbf4d7d4bcd4ba788d97864 from: Georgiy Lebedev via: Vladislav Shpilevoy date: Mon Aug 26 19:46:42 2024 UTC box: filter out tmp and remote txs when aborting all txs for DDL Currently, we lack support of DDL in transactions, so now we simply apply the strongest restriction and abort all other transactions when preparing a DDL transaction (8f4be32). However, we can relax this restriction for concurrent nop (particularly, those using fully-temporary spaces) and remote (applier) transactions, since they cannot possibly conflict each other by definition. There can be 2 cases: 1. If the `ddl_owner` uses only fully-temporary spaces, we can allow transactions containing only applier rows to continue. Since we do this in `engine_prepare`, we can be sure that `ddl`_owner will not have any other statements, while the transaction containing only applier rows, by definition, cannot use `fully-temporary` spaces, so there can never be a conflict. Regarding `before_replace` or `on_replace` triggers of the applier transactions: although no DDL can be used inside them, they can still use obsolete fully-temporary space definitions. However, if we see that an applier transaction contains only applier rows at the moment of the `ddl_owner` preparation, it is safe to let them continue, since later usage of the prepared fully-temporary space DDL changes by triggers of the applier transactions will be consistent. 2. If the `ddl_owner` contains only applier rows, we can allow transactions that use only fully-temporary spaces to continue. It is safe to do so, since later usage of the committed DDL changes by the in-progress transactions will be consistent. Both of these checks conform to this rule (8f4be32): > In fact we should only abort transactions that have read dirty (i.e. modified) objects. Closes #9720 NO_DOC= commit - 284cc37913fd528f08603942ef01b3726e38e260 commit + 6e1e1d50a703a26b4dbf4d7d4bcd4ba788d97864 blob - /dev/null blob + d31ddff0bb1fc0014e87ba6ec58d311e8c19fba6 (mode 644) --- /dev/null +++ changelogs/unreleased/gh-9720-tmp-spaces-ddl-abort-applier-txs.md @@ -0,0 +1,5 @@ +## bugfix/box + +* Now fully-temporary spaces DDL does not abort concurrent purely remote + (applier) transactions, and DDL in purely remote transactions does not abort + concurrent fully-temporary spaces transactions (gh-9720). blob - c4cc9a427906e7812a5b26eec14963570aa8ec73 blob + f098fd6b477035901312572d011a15bc3fa71fa5 --- src/box/memtx_space.c +++ src/box/memtx_space.c @@ -1073,6 +1073,13 @@ memtx_space_check_format(struct space *space, struct t ERROR_INJECT_YIELD(ERRINJ_CHECK_FORMAT_DELAY); + ERROR_INJECT_COUNTDOWN(ERRINJ_CHECK_FORMAT_DELAY_COUNTDOWN, { + struct errinj *e = + errinj(ERRINJ_CHECK_FORMAT_DELAY, ERRINJ_BOOL); + e->bparam = true; + ERROR_INJECT_YIELD(ERRINJ_CHECK_FORMAT_DELAY); + }); + tuple_unref(state.cursor); if (state.rc != 0) { rc = -1; blob - 7d6608cfa793a1c74307a5705f786b923b1c4961 blob + 5f8f5e079cd7cb7e53e95316ccc3dd12f47b925b --- src/box/memtx_tx.c +++ src/box/memtx_tx.c @@ -681,6 +681,19 @@ memtx_tx_acquire_ddl(struct txn *tx) { tx->is_schema_changed = true; (void) txn_can_yield(tx, false); +} + +/** + * Fully temporary and remote transactions cannot possibly conflict each other + * by definition, so we can filter them out when aborting transactions for DDL. + * Temporary space transactions are a always nop, since they never have WAL rows + * associated with them. + */ +static bool +memtx_tx_filter_temporary_and_remote_txs(struct txn *txn1, struct txn *txn2) +{ + return (txn_is_fully_temporary(txn1) && txn_is_fully_remote(txn2)) || + (txn_is_fully_remote(txn1) && txn_is_fully_temporary(txn2)); } void @@ -695,6 +708,9 @@ memtx_tx_abort_all_for_ddl(struct txn *ddl_owner) if (to_be_aborted->status != TXN_INPROGRESS && to_be_aborted->status != TXN_IN_READ_VIEW) continue; + if (memtx_tx_filter_temporary_and_remote_txs(ddl_owner, + to_be_aborted)) + continue; to_be_aborted->status = TXN_ABORTED; txn_set_flags(to_be_aborted, TXN_IS_CONFLICTED); say_warn("Transaction committing DDL (id=%lld) has aborted " blob - 6801c4af77b4dab7b90bffa50d00699771d02875 blob + cf12f5459c5a5604190442fdf98e3a3fbd780c81 --- src/box/txn.c +++ src/box/txn.c @@ -663,6 +663,40 @@ txn_is_distributed(struct txn *txn) */ return (txn->n_new_rows > 0 && txn->n_applier_rows > 0 && txn->n_new_rows != txn->n_local_rows); +} + +bool +txn_is_fully_temporary(struct txn *txn) +{ + if (!txn_is_nop(txn)) + return false; + struct txn_stmt *stmt; + stailq_foreach_entry(stmt, &txn->stmts, next) { + if (stmt->space != NULL && + stmt->space->def->opts.type == SPACE_TYPE_DATA_TEMPORARY) + return false; + } + return true; +} + +bool +txn_is_fully_remote(struct txn *txn) +{ + if (txn->n_new_rows != 0) + return false; + struct txn_stmt *stmt; + /* + * Allow DDL on data-temporary spaces, since we allow only fully + * temporary transactions to continue. + */ + stailq_foreach_entry(stmt, &txn->stmts, next) { + if (stmt->space != NULL && + space_is_data_temporary(stmt->space)) { + assert(stmt->row == NULL); + return false; + } + } + return true; } /** blob - 4b88ecc3cd34513f4bd632ba652ca28c630221cd blob + a88f7361b0cad59d296b9dba6b95c20baaaa9782 --- src/box/txn.h +++ src/box/txn.h @@ -914,6 +914,20 @@ txn_is_nop(const struct txn *txn) { return txn->n_new_rows + txn->n_applier_rows == 0; } + +/** + * Check whether a transaction consists only of operations on fully temporary + * spaces. + */ +bool +txn_is_fully_temporary(struct txn *txn); + +/** + * Check whether a transaction consists only of remote operations coming from + * the applier. + */ +bool +txn_is_fully_remote(struct txn *txn); /** * Mark @a stmt as temporary by removing the associated stmt->row blob - 3871b0e5bcd1645be2946195aa60b500d7c8677c blob + 82f853dd9a524864329f0441f077520a568cf197 --- src/box/vinyl.c +++ src/box/vinyl.c @@ -1150,6 +1150,12 @@ vinyl_space_check_format(struct space *space, struct t if (++loops % VY_YIELD_LOOPS == 0) fiber_sleep(0); ERROR_INJECT_YIELD(ERRINJ_CHECK_FORMAT_DELAY); + ERROR_INJECT_COUNTDOWN(ERRINJ_CHECK_FORMAT_DELAY_COUNTDOWN, { + struct errinj *e = + errinj(ERRINJ_CHECK_FORMAT_DELAY, ERRINJ_BOOL); + e->bparam = true; + ERROR_INJECT_YIELD(ERRINJ_CHECK_FORMAT_DELAY); + }); if (ctx.is_failed) { diag_move(&ctx.diag, diag_get()); rc = -1; blob - 78cb42491c92186a20fc220bf22da11f74a217ea blob + 22a2ae18e69c657c9128668356d0f301380a88f3 --- src/lib/core/errinj.h +++ src/lib/core/errinj.h @@ -83,6 +83,7 @@ struct errinj { _(ERRINJ_BUILD_INDEX_ON_ROLLBACK_ALLOC, ERRINJ_BOOL, {.bparam = false}) \ _(ERRINJ_BUILD_INDEX_TIMEOUT, ERRINJ_DOUBLE, {.dparam = 0}) \ _(ERRINJ_CHECK_FORMAT_DELAY, ERRINJ_BOOL, {.bparam = false}) \ + _(ERRINJ_CHECK_FORMAT_DELAY_COUNTDOWN, ERRINJ_INT, {.iparam = -1}) \ _(ERRINJ_COIO_SENDFILE_CHUNK, ERRINJ_INT, {.iparam = -1}) \ _(ERRINJ_COIO_WRITE_CHUNK, ERRINJ_BOOL, {.bparam = false}) \ _(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT, {.iparam = 0}) \ blob - /dev/null blob + cb180a9608a93e451d0356cc22285e5238e080ca (mode 644) --- /dev/null +++ test/replication-luatest/gh_9720_tmp_spaces_ddl_abort_applier_txs_test.lua @@ -0,0 +1,276 @@ +local t = require('luatest') +local replica_set = require('luatest.replica_set') +local server = require('luatest.server') + +local g = t.group(nil, t.helpers.matrix({memtx_use_mvcc = {false, true}})) + +g.before_each(function(cg) + cg.replica_set = replica_set:new{} + local box_cfg = { + memtx_use_mvcc_engine = cg.params.memtx_use_mvcc, + replication = { + server.build_listen_uri('server1', cg.replica_set.id), + server.build_listen_uri('server2', cg.replica_set.id), + }, + replication_timeout = 0.1, + } + cg.server1 = cg.replica_set:build_and_add_server{ + alias = 'server1', + box_cfg = box_cfg, + } + box_cfg.read_only = true + cg.server2 = cg.replica_set:build_and_add_server{ + alias = 'server2', + box_cfg = box_cfg, + } + cg.replica_set:start() + cg.server1:exec(function() + local s = box.schema.create_space('s') + s:create_index('p') + box.begin() + for i = 1, 1000 do + s:replace{i, i} + end + box.commit() + end) + cg.server2:exec(function() + box.cfg{read_only = false} + end) + cg.replica_set:wait_for_fullmesh() + cg.server1:wait_for_downstream_to(cg.server2) +end) + +g.after_each(function(cg) + cg.replica_set:drop() +end) + +-- Test that a DDL transaction on fully temporary spaces does not abort a remote +-- transaction. +g.test_fully_temporary_ddl_does_not_abort_remote_tx = function(cg) + t.tarantool.skip_if_not_debug() + + cg.server2:exec(function() + box.error.injection.set('ERRINJ_CHECK_FORMAT_DELAY_COUNTDOWN', 0) + end) + cg.server1:exec(function() + box.space.s:format{'i', 'unsigned'} + end) + cg.server2:exec(function() + t.helpers.retrying({timeout = 120}, function() + t.assert(box.error.injection.get('ERRINJ_CHECK_FORMAT_DELAY', true)) + end) + box.schema.space.create('tmp', {type = 'temporary'}) + box.error.injection.set('ERRINJ_CHECK_FORMAT_DELAY', false) + end) + cg.server1:wait_for_downstream_to(cg.server2) + cg.server2:exec(function() + local msg = "Transaction has been aborted by conflict" + t.assert_not_equals(box.info.replication[1].upstream.message, msg) + t.assert_not_equals(box.space.s:format(), {}) + end) +end + +-- Test that a DDL transaction on fully temporary spaces still aborts a not +-- fully remote transaction. +g.test_fully_temporary_ddl_aborts_not_fully_remote_tx = function(cg) + t.skip_if(not cg.params.memtx_use_mvcc) + + cg.server2:exec(function() + local tmp = box.schema.space.create('tmp-replace', {type = 'temporary'}) + tmp:create_index('p') + + box.space.s:on_replace(function() + tmp:replace{0} + rawset(_G, 'executed_on_replace', true) + require('fiber').sleep(60) + end) + end) + cg.server1:exec(function() + box.space.s:replace{0} + end) + cg.server2:exec(function() + t.helpers.retrying({timeout = 60}, function() + t.assert(_G.executed_on_replace) + end) + box.schema.space.create('tmp', {type = 'temporary'}) + end) + local msg = "Transaction committing DDL %(id=%d+%) has aborted " .. + "another TX %(id=%d+%)" + t.assert(cg.server2:grep_log(msg)) + cg.server2:exec(function() + t.assert_equals(box.space.s:get{0}, nil) + end) +end + +-- Test that a DDL transaction on fully temporary spaces does not abort a +-- fully remote transaction that rollbacked to a savepoint without local +-- changes. +g.test_fully_temporary_ddl_does_not_abort_fully_remote_tx_after_rb_to_svp = +function(cg) + t.skip_if(not cg.params.memtx_use_mvcc) + + cg.server2:exec(function() + local tmp = box.schema.space.create('tmp-replace', {type = 'temporary'}) + tmp:create_index('p') + + box.space.s:on_replace(function() + local svp = box.savepoint() + tmp:replace{0} + box.rollback_to_savepoint(svp) + rawset(_G, 'executed_on_replace', true) + t.helpers.retrying({timeout = 120}, function() + t.assert(_G.can_leave_on_replace) + end) + end) + end) + cg.server1:exec(function() + box.space.s:replace{0} + end) + cg.server2:exec(function() + t.helpers.retrying({timeout = 120}, function() + t.assert(_G.executed_on_replace) + end) + box.schema.space.create('tmp', {type = 'temporary'}) + rawset(_G, 'can_leave_on_replace', true) + end) + cg.server1:wait_for_downstream_to(cg.server2) + cg.server2:exec(function() + t.assert_not_equals(box.space.s:get{0}, nil) + end) +end + +-- Test that a remote DDL transaction does not abort a transaction on fully +-- temporary spaces. +g.test_fully_remote_ddl_does_not_abort_fully_temporary_tx = function(cg) + t.tarantool.skip_if_not_debug() + + local fid = cg.server2:exec(function() + local tmp = box.schema.space.create('tmp', {type = 'temporary'}) + tmp:create_index('p') + box.begin() + for i = 1, 1000 do + tmp:replace{i, i} + end + box.commit() + box.error.injection.set('ERRINJ_CHECK_FORMAT_DELAY', true) + local f = require('fiber').new(function() + tmp:format{'i', 'unsigned'} + end) + f:set_joinable(true) + return f:id() + end) + cg.server1:exec(function() + box.schema.space.create('ss') + end) + cg.server1:wait_for_downstream_to(cg.server2) + cg.server2:exec(function(fid) + box.error.injection.set('ERRINJ_CHECK_FORMAT_DELAY', false) + local ok = require('fiber').find(fid):join() + t.assert(ok) + t.assert_not_equals(box.space.tmp:format(), {}) + end, {fid}) +end + +-- Test that a remote DDL transaction still aborts a not fully temporary +-- transaction. +g.test_fully_remote_tx_ddl_aborts_not_fully_temporary_tx = function(cg) + t.skip_if(not cg.params.memtx_use_mvcc) + + local fid = cg.server2:exec(function() + local fiber = require('fiber') + + local tmp = box.schema.space.create('tmp-replace', {type = 'temporary'}) + tmp:create_index('p') + + rawset(_G, 'downstream_cond', fiber.cond()) + local f = fiber.new(function() + box.begin() + tmp:replace{0} + box.space.s:replace{0} + _G.downstream_cond:wait() + box.commit() + end) + f:set_joinable(true) + return f:id() + end) + cg.server1:exec(function() + box.schema.space.create('ss') + end) + cg.server1:wait_for_downstream_to(cg.server2) + cg.server2:exec(function(fid) + _G.downstream_cond:signal() + local ok, err = require('fiber').find(fid):join() + t.assert_not(ok) + t.assert_equals(err.message, "Transaction has been aborted by conflict") + end, {fid}) +end + +-- Test that a remote DDL transaction does not abort a fully temporary +-- transaction that rolled back to a savepoint without non-temporary changes. +g.test_fully_remote_tx_ddl_does_not_abort_fully_temporary_tx_after_rb_to_svp = +function(cg) + t.skip_if(not cg.params.memtx_use_mvcc) + + local fid = cg.server2:exec(function() + local fiber = require('fiber') + + local tmp = box.schema.space.create('tmp-replace', {type = 'temporary'}) + tmp:create_index('p') + + rawset(_G, 'downstream_cond', fiber.cond()) + local f = fiber.new(function() + box.begin() + tmp:replace{0} + local svp = box.savepoint() + box.space.s:replace{0} + box.rollback_to_savepoint(svp) + _G.downstream_cond:wait() + box.commit() + end) + f:set_joinable(true) + return f:id() + end) + cg.server1:exec(function() + box.schema.space.create('ss') + end) + cg.server1:wait_for_downstream_to(cg.server2) + cg.server2:exec(function(fid) + _G.downstream_cond:signal() + local ok = require('fiber').find(fid):join() + t.assert(ok) + end, {fid}) +end + +-- Test that a fully remote DDL transaction still aborts a data-temporary +-- transaction. +g.test_fully_remote_tx_ddl_aborts_data_temporary_tx = function(cg) + t.skip_if(not cg.params.memtx_use_mvcc) + + local fid = cg.server2:exec(function() + local fiber = require('fiber') + + local tmp = box.schema.space.create('data-tmp', + {type = 'data-temporary'}) + tmp:create_index('p') + + rawset(_G, 'downstream_cond', fiber.cond()) + local f = fiber.new(function() + box.begin() + tmp:replace{0} + _G.downstream_cond:wait() + box.commit() + end) + f:set_joinable(true) + return f:id() + end) + cg.server1:exec(function() + box.schema.space.create('ss') + end) + cg.server1:wait_for_downstream_to(cg.server2) + cg.server2:exec(function(fid) + _G.downstream_cond:signal() + local ok, err = require('fiber').find(fid):join() + t.assert_not(ok) + t.assert_equals(err.message, "Transaction has been aborted by conflict") + end, {fid}) +end