Re: 0f0ef93c4d4: MDEV-32444 Data from orphaned XA transaction is lost after online alter
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
Hello Sergei! On Thu, 19 Oct 2023 at 23:45, Sergei Golubchik <serg@mariadb.org> wrote:
--- 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.
Sure check out commit 06cc655a55d115, which extends XID instead. I left it separate to be easier readable, as it contains many changes that just practically fix the build issue, that would noise up the subj commit. 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.
I think extending XID was a better alternative. I didn't read the XA protocol: as far as I understand, only xid_t is its part, and XID is our alias -- or, well, not an alias anymore. Having it as an extension of xid_t looks quite natural to me. Besides, even the handlerton abi is left compatible.
+ 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?
that's what's called by the end of the statement in case of an XA transaction😐 -- Yours truly, Nikita Malyavin
Hi, Nikita, On Oct 20, Nikita Malyavin wrote:
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.
I think extending XID was a better alternative. I didn't read the XA protocol: as far as I understand, only xid_t is its part, and XID is our alias -- or, well, not an alias anymore. Having it as an extension of xid_t looks quite natural to me. Besides, even the handlerton abi is left compatible.
I had a link to a different XA implementation in my previous email. Here it is again: https://github.com/berkeleydb/libdb/blob/master/src/dbinc/xa.h There you can see ============= struct xid_t { long formatID; /* format identifier */ long gtrid_length; /* value from 1 through 64 */ long bqual_length; /* value from 1 through 64 */ char data[XIDDATASIZE]; }; typedef struct xid_t XID; ============= This is a verbatim copy from the standard. You can also search github for "path:/xa.h" and you'll find many matches all having the above declaration exactly. Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Alright! Copy that. Then perhaps I will better leave the semantic compatibility of the handlerton's callback field, and make a conversion on the plugin's side. -- Yours truly, Nikita Malyavin
Hi, I have made a separate struct XA_data to store online_alter_cache: https://github.com/MariaDB/server/commit/a72605ce002897c65d184b476eeec51a1d1... I was thinking on whether to change the declaration in handlerton - this would require may changes. Besides that: int innobase_commit_by_xid( /*===================*/ handlerton* hton, XID* xid) /*!< in: X/Open XA transaction identification */ Innodb expects the second argument to be XID*, I suppose that many other engines do, too, so I don't want to make it less clear what the second argument is. I also decided not to leave the comment for the commit/rollback callbacks, since there's no use for this extension to the other engines apart from online alter, in the form it is done now. -- Yours truly, Nikita Malyavin
Hi, Nikita, On Oct 24, Nikita Malyavin wrote:
I have made a separate struct XA_data to store online_alter_cache: https://github.com/MariaDB/server/commit/a72605ce002897c65d184b476eeec51a1d1...
Please, try not to forget to change the issue status in Jira to IN-REVIEW. I would've reviewed it a week ago if I'd seen it in my queue.
I was thinking on whether to change the declaration in handlerton - this would require may changes. Besides that:
int innobase_commit_by_xid( /*===================*/ handlerton* hton, XID* xid) /*!< in: X/Open XA transaction identification */
Innodb expects the second argument to be XID*, I suppose that many other engines do, too, so I don't want to make it less clear what the second argument is.
Totally agree. Had you changed it to XA_data, I would've asked you to revert it. The whole point of having a standard definition is interoperability. It makes zero sense to have a standard definition and not use it when communicating with other engines. I had a couple small-ish comments, see below. And, please, squash both commits into one before pushing.
diff --git a/sql/online_alter.cc b/sql/online_alter.cc index 8b8c81319d1..03622106bb3 100644 --- a/sql/online_alter.cc +++ b/sql/online_alter.cc @@ -334,14 +336,63 @@ 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; + };
I'd say that all these multi-line functions can be declared the old way. lambdas aren't helping here anymore (except for recover callback).
+ 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); + } + + cleanup_tables(thd); + return res; + }; + online_alter_hton->commit_by_xid= [](handlerton *hton, XID *x) -> int + { + auto *xid= static_cast<XA_data*>(x); + if (likely(xid->online_alter_cache == NULL)) + return 0; + 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 *x) -> int + { + auto *xid= static_cast<XA_data*>(x); + if (likely(xid->online_alter_cache == NULL)) + return 0; + 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 diff --git a/sql/xa.cc b/sql/xa.cc index 234d2a6dccc..97a62f0f095 100644 --- a/sql/xa.cc +++ b/sql/xa.cc @@ -140,6 +140,7 @@ class XID_cache_element { XID_cache_element *element= (XID_cache_element*) (ptr + LF_HASH_OVERHEAD); element->m_state= 0; + new(&element->xid) XID();
why is that? XID::XID isn't doing anything
} static void lf_alloc_destructor(uchar *ptr) {
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Hi! On Mon, 30 Oct 2023 at 13:57, Sergei Golubchik <serg@mariadb.org> wrote:
I'd say that all these multi-line functions can be declared the old way.
Have no objections. Extracted them as standalone static functions.
diff --git a/sql/xa.cc b/sql/xa.cc index 234d2a6dccc..97a62f0f095 100644 --- a/sql/xa.cc +++ b/sql/xa.cc @@ -140,6 +140,7 @@ class XID_cache_element { XID_cache_element *element= (XID_cache_element*) (ptr + LF_HASH_OVERHEAD); element->m_state= 0; + new(&element->xid) XID();
why is that? XID::XID isn't doing anything
The field initialization was missing after I made XA_data. Probably that's why some tests were failing after that update. -- Yours truly, Nikita Malyavin
participants (2)
-
Nikita Malyavin -
Sergei Golubchik