Hi, Nikita! On Nov 14, Nikita Malyavin wrote:
revision-id: a5fca9a6e30 (mariadb-10.5.2-478-ga5fca9a6e30) parent(s): ad6e7b87107 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2021-01-27 17:28:05 +1000 message:
MENT-651 [6/8] store cache managers in a list
again, a couple of lines with a description would be good here
diff --git a/mysql-test/suite/binlog/t/online_alter.test b/mysql-test/suite/binlog/t/online_alter.test index 804e847d008..4ed2db74bd6 100644 --- a/mysql-test/suite/binlog/t/online_alter.test +++ b/mysql-test/suite/binlog/t/online_alter.test @@ -218,7 +219,7 @@ set autocommit = 0; start transaction; update t1 set b= 7007 where a = 7; --error ER_DUP_ENTRY -update t1 set a= 8 where a = 8 or a = 9; +update t1 set a= 8, b= 8008 where a = 8 or a = 9 order by a;
why did you change tests for what is, as far as I understand, just an optimization?
commit; set autocommit = 1;
diff --git a/sql/handler.cc b/sql/handler.cc index 8b95653e69f..8b22167e729 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -6660,7 +6660,16 @@ static int binlog_log_row_online_alter(TABLE* table, THD *thd= table->in_use;
if (!table->online_alter_cache) - table->online_alter_cache= thd->binlog_setup_cache_data(); + { + auto *cache_mngr= online_alter_binlog_get_cache_mngr(thd, table); + // Use transaction cache directly, if it is not multi-transaction mode + table->online_alter_cache= binlog_get_cache_data(cache_mngr, + !thd->in_multi_stmt_transaction_mode()); + + trans_register_ha(thd, false, binlog_hton, 0); + if (thd->in_multi_stmt_transaction_mode()) + trans_register_ha(thd, true, binlog_hton, 0);
I still don't understand that. Why do you need a handlerton, why do you fake a transaction? And why do you need to search a list in THD, instead of storing cache_mngr in TABLE_SHARE?
+ }
// We need to log all columns for the case if alter table changes primary key. table->use_all_columns(); diff --git a/sql/log.cc b/sql/log.cc index 6c678624230..c58096e6d9a 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -2258,14 +2298,18 @@ static int binlog_rollback(handlerton *hton, THD *thd, bool all) { DBUG_ENTER("binlog_rollback");
- for (TABLE *table= thd->open_tables; table; table= table->next) - { - if (!table->online_alter_cache) - continue; - table->online_alter_cache->reset(); - delete table->online_alter_cache; - table->online_alter_cache= NULL; - } + bool is_ending_trans= ending_trans(thd, all); + + /* + This is a crucial moment that we are running through + thd->online_alter_cache_list, and not through thd->open_tables to cleanup + stmt cache, though both have it. The reason is that the tables can be closed + to that moment in case of an error. + The same reason applies to the fact we don't store cache_mngr in the table + itself -- because it can happen to be not existing. + Still in case if tables are left opened + */ + binlog_online_alter_cleanup(thd->online_alter_cache_list, is_ending_trans);
still don't understand that comment either :( Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org