Hi, Nikita, ok to push. spelling and renaming suggestions below On Nov 13, Nikita Malyavin wrote:

revision-id: 73097e18b77 (mariadb-11.2.1-20-g73097e18b77) parent(s): f7646d890b9 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2023-11-13 15:28:19 +0100 message:

MDEV-32771 Server crash upon online alter with concurrent XA

In case of a non-recovery XA rollback/commit in the same connection, thon->rollback is called instead of rollback_by_xid, Though reviously,

"previously"

thd_ha_data was moved to thd->transaction->xid_state.xid in hton->prepare.

Like it wasn't enough, XA PREPARE can be skipped upon user and thus we can end up in hton->commit/rollback with and unprepared XA, so checking xid_state.is_explicit_XA is not enough -- we should check xid_state.get_state_code() == XA_PREPARED, which will also guarantee is_explicit_XA() == true.

--- a/sql/online_alter.cc +++ b/sql/online_alter.cc @@ -315,17 +315,35 @@ int online_alter_savepoint_rollback(handlerton *hton, THD *thd, sv_id_t sv_id)

static int online_alter_commit(handlerton *hton, THD *thd, bool all) { - int res= online_alter_end_trans(get_cache_list(hton, thd), thd, - ending_trans(thd, all), true); - cleanup_tables(thd); + int res; + bool commit= ending_trans(thd, all); + if (thd->transaction->xid_state.get_state_code() == XA_PREPARED && commit) + { + res= hton->commit_by_xid(hton, thd->transaction->xid_state.get_xid()); + // cleanup is done by prepare()

this is conusing, there's no prepare after commit_by_xid. may be you wanted to say "cleanup was already done by prepare()" ?

+ } + else + { + res= online_alter_end_trans(get_cache_list(hton, thd), thd, commit, true); + cleanup_tables(thd); + } return res; };

static int online_alter_rollback(handlerton *hton, THD *thd, bool all) { - int res= online_alter_end_trans(get_cache_list(hton, thd), thd, - ending_trans(thd, all), false); - cleanup_tables(thd); + int res; + bool commit= ending_trans(thd, all);

'commit' is weird here, on the rollback path. may be you rename it to, like "is_ending_tran" or "end_tran" or "is_persistent" or something along these lines. And then 'commit' in online_alter_commit too, for consistency.

+ if (thd->transaction->xid_state.get_state_code() == XA_PREPARED && commit) + { + res= hton->rollback_by_xid(hton, thd->transaction->xid_state.get_xid()); + // cleanup is done by prepare()

same as in online_alter_commit

+ } + else + { + res= online_alter_end_trans(get_cache_list(hton, thd), thd, commit, false); + cleanup_tables(thd); + } return res; };

Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org