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 secur...@mariadb.org
_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-le...@lists.mariadb.org
