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