maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx