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