Hi, Nikita, See below. A small suggestion for the test. And a rather larger comment about extending xid_t, as you, likely, expected :) On Oct 19, Nikita Malyavin wrote:
revision-id: 0f0ef93c4d4 (mariadb-11.2.1-15-g0f0ef93c4d4) parent(s): 7f00853f2f1 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2023-10-13 13:39:36 +0400
diff --git a/mysql-test/main/alter_table_online_debug.result b/mysql-test/main/alter_table_online_debug.result index 1ebb19965eb..154aec003ca 100644 --- a/mysql-test/main/alter_table_online_debug.result +++ b/mysql-test/main/alter_table_online_debug.result @@ -1563,6 +1563,103 @@ connection default; drop table t1, t2; set @@binlog_format=default; set debug_sync= reset; +# MDEV-32444 Data from orphaned XA transaction is lost after online alter +create table t (a int primary key) engine=innodb; +insert into t values (1); +# XA commit +set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for go'; +alter table t force, algorithm=copy, lock=none; +connection con1; +set debug_sync= 'now wait_for downgraded'; +xa begin 'x1'; +update t set a = 2 where a = 1; +xa end 'x1'; +xa prepare 'x1'; +disconnect con1; +connection con2; +xa commit 'x1'; +set debug_sync= 'now signal go'; +connection default; +select * from t; +a +2 +# XA rollback +set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for go'; +alter table t force, algorithm=copy, lock=none; +connect con1, localhost, root,,; +xa begin 'x2'; +insert into t values (53); +xa end 'x2'; +xa prepare 'x2'; +disconnect con1; +connection con2; +xa rollback 'x2'; +set debug_sync= 'now signal go'; +connection default; +select * from t; +a +2 +# XA transaction is left uncommitted +# end then is rollbacked after alter fails +set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for go'; +set statement innodb_lock_wait_timeout=0, lock_wait_timeout= 0 +for alter table t force, algorithm=copy, lock=none; +connect con1, localhost, root,,; +xa begin 'xuncommitted'; +insert into t values (3); +xa end 'xuncommitted'; +xa prepare 'xuncommitted'; +set debug_sync= 'now signal go'; +disconnect con1; +connection default; +ERROR HY000: Lock wait timeout exceeded; try restarting transaction +xa rollback 'xuncommitted'; +select * from t; +a +2 +# Same, but commit +set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for go'; +set statement innodb_lock_wait_timeout=0, lock_wait_timeout= 0 +for alter table t force, algorithm=copy, lock=none;
instead of `alter table t force`, better do something that changes the table, so that you could see whether alter was executed or aborted. For example, you can change the column type int->bigint and back. not only here, but everywhere. Or add/drop column, this has an added benefit that you won't need a separate `show create table`, your already existing `select * from t` will show whether the extra column exists
+connect con1, localhost, root,,; +xa begin 'committed_later'; +insert into t values (3); +xa end 'committed_later'; +xa prepare 'committed_later'; +set debug_sync= 'now signal go'; +disconnect con1; +connection default; +ERROR HY000: Lock wait timeout exceeded; try restarting transaction +xa commit 'committed_later'; +select * from t; +a +2 +3 +# Commit, but error in statement, and there is some stmt data to rollback +set debug_sync= 'alter_table_online_downgraded signal downgraded wait_for go'; +alter table t force, algorithm=copy, lock=none; +connect con1, localhost, root,,; +set debug_sync= 'now wait_for downgraded'; +xa begin 'x1'; +insert into t values (4), (3); +ERROR 23000: Duplicate entry '3' for key 'PRIMARY' +insert into t values (5); +xa end 'x1'; +xa prepare 'x1'; +disconnect con1; +connection con2; +xa commit 'x1'; +set debug_sync= 'now signal go'; +connection default; +select * from t; +a +2 +3 +5 +connect con1, localhost, root,,; +connection default; +drop table t; +set debug_sync= reset; disconnect con1; disconnect con2; # diff --git a/sql/handler.h b/sql/handler.h index 07b392c010a..4622e49f883 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -903,6 +905,7 @@ struct xid_t { long gtrid_length; long bqual_length; char data[XIDDATASIZE]; // not \0-terminated ! + Online_alter_cache_list *online_alter_cache;
well, no, we've talked about it. Even the comment above the structure says that you cannot change it. make a separate structure, xid_and_online_alter_cache_t or xid_t_internal or whatever. but xid_t should be defined identically everywhere. For example: https://github.com/berkeleydb/libdb/blob/master/src/dbinc/xa.h === okay, now I've seen how you used it and I'd say it's _such a rare use case_ that you can store your online_alter_cache in the XID_cache_element and look it up there. Or even in a separate data structure, that will be empty most of the time anyway.
xid_t() = default; /* Remove gcc warning */ bool eq(struct xid_t *xid) const diff --git a/sql/online_alter.cc b/sql/online_alter.cc index 8b8c81319d1..19abe57914d 100644 --- a/sql/online_alter.cc +++ b/sql/online_alter.cc @@ -334,14 +336,57 @@ static int online_alter_log_init(void *p) online_alter_hton->savepoint_rollback_can_release_mdl= [](handlerton *hton, THD *thd){ return true; };
- online_alter_hton->commit= [](handlerton *hton, THD *thd, bool all) - { return online_alter_end_trans(hton, thd, all, true); }; - online_alter_hton->rollback= [](handlerton *hton, THD *thd, bool all) - { return online_alter_end_trans(hton, thd, all, false); }; - online_alter_hton->commit_by_xid= [](handlerton *hton, XID *xid) - { return online_alter_end_trans(hton, current_thd, true, true); }; - online_alter_hton->rollback_by_xid= [](handlerton *hton, XID *xid) - { return online_alter_end_trans(hton, current_thd, true, false); }; + online_alter_hton->commit= [](handlerton *hton, THD *thd, bool all) -> int + { + int res= online_alter_end_trans(get_cache_list(hton, thd), thd, + ending_trans(thd, all), true); + cleanup_tables(thd); + return res; + }; + online_alter_hton->rollback= [](handlerton *hton, THD *thd, bool all) -> int + { + int res= online_alter_end_trans(get_cache_list(hton, thd), thd, + ending_trans(thd, all), false); + cleanup_tables(thd); + return res; + }; + + + online_alter_hton->recover= [](handlerton*, XID*, uint){ return 0; }; + online_alter_hton->prepare= [](handlerton *hton, THD *thd, bool all) -> int + { + auto &cache_list= get_cache_list(hton, thd); + int res= 0; + if (ending_trans(thd, all)) + { + thd->transaction->xid_state.set_online_alter_cache(&cache_list); + thd_set_ha_data(thd, hton, NULL); + } + else + { + res= online_alter_end_trans(cache_list, thd, false, true);
is that possible?
+ } + + cleanup_tables(thd); + return res; + }; + online_alter_hton->commit_by_xid= [](handlerton *hton, XID *xid) -> int + { + int res= online_alter_end_trans(*xid->online_alter_cache, current_thd, + true, true); + delete xid->online_alter_cache; + xid->online_alter_cache= NULL; + return res; + }; + online_alter_hton->rollback_by_xid= [](handlerton *hton, XID *xid) -> int + { + int res= online_alter_end_trans(*xid->online_alter_cache, current_thd, + true, false); + delete xid->online_alter_cache; + xid->online_alter_cache= NULL; + return res; + }; +
online_alter_hton->drop_table= [](handlerton *, const char*) { return -1; }; online_alter_hton->flags= HTON_NOT_USER_SELECTABLE | HTON_HIDDEN
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org