Hi, Nikita, On Dec 13, Nikita Malyavin wrote:

...

...

-class binlog_cache_mngr { +class binlog_cache_mngr: public ilist_node<> { public: binlog_cache_mngr(my_off_t param_max_binlog_stmt_cache_size, my_off_t param_max_binlog_cache_size, ulong *param_ptr_binlog_stmt_cache_use, ulong *param_ptr_binlog_stmt_cache_disk_use, ulong *param_ptr_binlog_cache_use, - ulong *param_ptr_binlog_cache_disk_use) + ulong *param_ptr_binlog_cache_disk_use, + TABLE_SHARE *share) - : last_commit_pos_offset(0), using_xa(FALSE), xa_xid(0) + : last_commit_pos_offset(0), using_xa(FALSE), xa_xid(0), share(share)

does it work, can one do it? I mean share(share) in other places, as far as I remember, arguments always use a distinct name, like share(share_arg) or share(share_) or something.

Yes, this is a language feature for constructor initializers.

Okay. We don't use it though and it will stop almost everyone looking at this code, as it's all too easy to assume it's a mistake. So I'd recommend to rename the argument to avoid misunderstanding. Like, to `share_`. But also feel free to keep it your way, it's a valid C++ after all.

...

...

{ stmt_cache.set_binlog_cache_info(param_max_binlog_stmt_cache_size, param_ptr_binlog_stmt_cache_use, @@ -2116,9 +2144,58 @@ int binlog_commit(THD *thd, bool all, bool ro_1pc) ... + else + { + error= cache_copy(&cache_mngr.trx_cache.cache_log, + &cache_mngr.stmt_cache.cache_log);

Why do you need two caches here? It seems you could have just one trx_cache and if the statement fails you simply truncate it to the beginning of the statement.

Didn't think about that, but seems so. Why would binlog need two caches then?

I don't really know. But, for example, I can think of a multi-table update that modifies both InnoDB and MyISAM tables and then fails after updating some rows. It'll be reverted in InnoDB, but not in MyISAM. With two caches InnoDB row events can go into stmt_cache and MyISAM row events can go into trx_cache. And later stmt_cache will be discarded, but MyISAM changes will persist. This multi-table case does not apply to online alter. Are there other cases when a separate stmt_cache might be needed?

...

...

+ binlog_cache_mngr *const cache_mngr= thd->binlog_get_cache_mngr(); + /* + cache_mngr can be NULL in case if binlog logging is disabled. + */ if (!cache_mngr) { DBUG_ASSERT(WSREP(thd) ||

Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org