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