Re: ad6a305d84d: online alter: use thd->ha_data to store cache_list
Hi, Nikita, On Sep 29, Nikita Malyavin wrote:
revision-id: ad6a305d84d (mariadb-11.2.1-9-gad6a305d84d) parent(s): 132c56f1325 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2023-09-23 11:46:36 +0400 message:
online alter: use thd->ha_data to store cache_list
nice
diff --git a/sql/online_alter.cc b/sql/online_alter.cc index 3255fc06c3e..5ad2156cd21 100644 --- a/sql/online_alter.cc +++ b/sql/online_alter.cc @@ -6,6 +6,7 @@
static handlerton *online_alter_hton;
+using Online_alter_cache_list= ilist<online_alter_cache_data>;
please, use typedef for that
class online_alter_cache_data: public Sql_alloc, public ilist_node<>, public binlog_cache_data @@ -246,7 +268,11 @@ int online_alter_savepoint_rollback(THD *thd, LEX_CSTRING name)
static int online_alter_close_connection(handlerton *hton, THD *thd) { - DBUG_ASSERT(thd->online_alter_cache_list.empty()); + Ha_data &ha= thd->ha_data[online_alter_hton->slot]; + auto *cache_list= (Online_alter_cache_list*)ha.ha_ptr;
why not thd_get_ha_data()? and don't forget thd_set_ha_data(thd, online_alter_hton, 0); THD can be reused, I suppose, so let's play safe.
+ + DBUG_ASSERT(!cache_list || cache_list->empty()); + delete cache_list; return 0; }
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Hi! On Fri, 29 Sept 2023 at 21:41, Sergei Golubchik <serg@mariadb.org> wrote:
+using Online_alter_cache_list= ilist<online_alter_cache_data>;
please, use typedef for that
Fine, but can you explain what's your reasoning behind this?
class online_alter_cache_data: public Sql_alloc, public ilist_node<>,
public binlog_cache_data @@ -246,7 +268,11 @@ int online_alter_savepoint_rollback(THD *thd,
LEX_CSTRING name)
static int online_alter_close_connection(handlerton *hton, THD *thd) { - DBUG_ASSERT(thd->online_alter_cache_list.empty()); + Ha_data &ha= thd->ha_data[online_alter_hton->slot]; + auto *cache_list= (Online_alter_cache_list*)ha.ha_ptr;
why not thd_get_ha_data()?
I suppose that this simple wrapper is used for dependency inversion, i.e. to access thd data outside `sql` scope, like innodb does it. -- Yours truly, Nikita Malyavin
Hi, Nikita, On Oct 09, Nikita Malyavin wrote:
Hi!
On Fri, 29 Sept 2023 at 21:41, Sergei Golubchik wrote:
+using Online_alter_cache_list= ilist<online_alter_cache_data>;
please, use typedef for that
Fine, but can you explain what's your reasoning behind this?
I've never seen this pattern before, so I googled. It seems to be exactly the same as typedef, is that right? In that case, it's just an Occam's razor - we use typedef everywhere already, we never use using for this purpose, so why multiply entities beyond necessity? Or am I wrong and using has a different semantics in this context?
class online_alter_cache_data: public Sql_alloc, public ilist_node<>,
public binlog_cache_data @@ -246,7 +268,11 @@ int online_alter_savepoint_rollback(THD *thd, LEX_CSTRING name)
static int online_alter_close_connection(handlerton *hton, THD *thd) { - DBUG_ASSERT(thd->online_alter_cache_list.empty()); + Ha_data &ha= thd->ha_data[online_alter_hton->slot]; + auto *cache_list= (Online_alter_cache_list*)ha.ha_ptr;
why not thd_get_ha_data()?
I suppose that this simple wrapper is used for dependency inversion, i.e. to access thd data outside `sql` scope, like innodb does it.
well, yes. it's also a convenient helper to access ha_data in sql/ too. you know, hide the implementation, this kind of things. why not to use it, for performance reasons? what's the downside? Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
On Mon, 9 Oct 2023 at 17:42, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita,
On Oct 09, Nikita Malyavin wrote:
Hi!
On Fri, 29 Sept 2023 at 21:41, Sergei Golubchik wrote:
+using Online_alter_cache_list= ilist<online_alter_cache_data>;
please, use typedef for that
Fine, but can you explain what's your reasoning behind this?
I've never seen this pattern before, so I googled. It seems to be exactly the same as typedef, is that right?
In that case, it's just an Occam's razor - we use typedef everywhere already, we never use using for this purpose, so why multiply entities beyond necessity?
Or am I wrong and using has a different semantics in this context?
I had an impression that using is "semantically better" somehow: * Something about that it doesn't require typename sometimes (but I googled now, it seems wrong * Also I thought that using expressions build faster. Can't find anything related as well now. So, ok, no problem, I removed it. I only saw a stylistic suggestion that using can be preferred when a symbol is brought into a local scope, while typedef is for externally used definitions. If we are going to always prefer typedefs over using, then it should be clarified in the code style, or otherwise resolved there.
class online_alter_cache_data: public Sql_alloc, public ilist_node<>,
public binlog_cache_data @@ -246,7 +268,11 @@ int online_alter_savepoint_rollback(THD *thd,
LEX_CSTRING name)
static int online_alter_close_connection(handlerton *hton, THD *thd) { - DBUG_ASSERT(thd->online_alter_cache_list.empty()); + Ha_data &ha= thd->ha_data[online_alter_hton->slot]; + auto *cache_list= (Online_alter_cache_list*)ha.ha_ptr;
why not thd_get_ha_data()?
I suppose that this simple wrapper is used for dependency inversion, i.e. to access thd data outside `sql` scope, like innodb does it.
well, yes. it's also a convenient helper to access ha_data in sql/ too. you know, hide the implementation, this kind of things.
why not to use it, for performance reasons? what's the downside?
We are kinda not used to hide a single array subscription behind a function call around the code base:) And what are we hiding? I suppose it's not thd->ha_data, but rather it is hton's ha_data, that is, ha_ptr, so the function should be called hton_get_ha_data, not thd. There can be some sense in hiding it, I suppose - if we'll ought to optimize the registry for many htons loaded into a system, and transform ha_data into a hash table, so probably it's better to prefere accessing it through the function. -- Yours truly, Nikita Malyavin
participants (2)
-
Nikita Malyavin -
Sergei Golubchik