← Back to team overview

maria-developers team mailing list archive

Re: MDEV-16329 ALTER ONLINE TABLE

 

Hi, Nikita,

this is a review of the combined diff of your branch.
Of `git diff github/10.8...github/bb-10.8-online-alter`, that is of

  cf7cc376bac fix savepoints in myisam
  4a128ca5458 fix main.delayed
  7300151cc84 fix skipping rocksdb
  19504dfdbcc add binlog/standalone combinations
  55e993595aa add rocksdb combination
  06645b17450 savepoints
  9ffcd13b9fe rename tests
  730926ae962 MDEV-16329 [5/5] ALTER ONLINE TABLE
  cf52ea33a70 MDEV-16329 [4/5] Refactor MYSQL_BIN_LOG: extract Event_log ancestor
  0445c8f1ee9 MDEV-16329 [3/5] use binlog_cache_data directly in most places
  3f3da0f65a3 MDEV-16329 [2/5] refactor binlog and cache_mngr
  dc79667ca73 MDEV-16329 [1/5] add THD::binlog_get_cache_mngr
  6f9dd00ce2d rpl: repack table_def
  faf8680df8d Copy_field: add const to arguments

It was mostly fine the last time already, so only few comments below:

> diff --git a/mysql-test/include/have_log_bin_off.require b/mysql-test/include/have_log_bin_off.require
> new file mode 100644
> index 00000000000..979dbe75f80
> --- /dev/null
> +++ b/mysql-test/include/have_log_bin_off.require
> @@ -0,0 +1,2 @@
> +Variable_name	Value
> +log_bin	OFF

better use `if (...) { skip }` pattern
*.require is an obsolete 20-year old technique from before if- and skip-times

> diff --git a/mysql-test/main/alter_table.test b/mysql-test/main/alter_table.test
> index 31c69783248..91423ee8c1f 100644
> --- a/mysql-test/main/alter_table.test
> +++ b/mysql-test/main/alter_table.test
> @@ -1532,8 +1532,12 @@ ALTER TABLE t1 DROP INDEX i1, DROP INDEX i2, DROP INDEX i3, DROP INDEX i4;
>  ALTER TABLE t1 ADD INDEX i1(b), ALGORITHM= INPLACE, LOCK= NONE;
>  ALTER TABLE t1 ADD INDEX i2(b), ALGORITHM= INPLACE, LOCK= SHARED;
>  ALTER TABLE t1 ADD INDEX i3(b), ALGORITHM= INPLACE, LOCK= EXCLUSIVE;
> ---error ER_ALTER_OPERATION_NOT_SUPPORTED_REASON
> +--disable_info
> +--disable_warnings
> +--error 0,ER_ALTER_OPERATION_NOT_SUPPORTED_REASON

why, because of embedded?

>  ALTER TABLE t1 ADD INDEX i4(b), ALGORITHM= COPY, LOCK= NONE;
> +--enable_warnings
> +--enable_info
>  ALTER TABLE t1 ADD INDEX i5(b), ALGORITHM= COPY, LOCK= SHARED;
>  ALTER TABLE t1 ADD INDEX i6(b), ALGORITHM= COPY, LOCK= EXCLUSIVE;
>  
> diff --git a/mysql-test/main/alter_table_locknone.result b/mysql-test/main/alter_table_locknone.result
> new file mode 100644
> index 00000000000..ea040925efe
> --- /dev/null
> +++ b/mysql-test/main/alter_table_locknone.result
> @@ -0,0 +1,270 @@
> +create table t1 (a int not null primary key, b int, c varchar(80), e enum('a','b')) engine=myisam;
> +insert into t1 (a) values (1),(2),(3);
> +alter online table t1 modify b int default 5, alter c set default 'X';
> +alter online table t1 change b new_name int;
> +alter online table t1 modify e enum('a','b','c');
> +alter online table t1 comment "new comment";
> +alter table t1 add constraint q check (a > 0);
> +alter online table t1 drop constraint q;
> +alter online table t1 algorithm=INPLACE, lock=NONE;
> +alter online table t1;
> +alter table t1 algorithm=INPLACE;
> +alter table t1 lock=NONE;
> +show create table t1;
> +Table	Create Table
> +t1	CREATE TABLE `t1` (
> +  `a` int(11) NOT NULL,
> +  `new_name` int(11) DEFAULT NULL,
> +  `c` varchar(80) DEFAULT 'X',
> +  `e` enum('a','b','c') DEFAULT NULL,
> +  PRIMARY KEY (`a`)
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1 COMMENT='new comment'
> +drop table t1;
> +create temporary table t1 (a int not null primary key, b int, c varchar(80), e enum('a','b'));
> +insert into t1 (a) values (1),(2),(3);
> +alter online table t1 modify b int default 5, alter c set default 'X';
> +alter online table t1 change b new_name int;
> +alter online table t1 modify e enum('a','b','c');
> +alter online table t1 comment "new comment";
> +alter online table t1 rename to t2;
> +show create table t2;
> +Table	Create Table
> +t2	CREATE TEMPORARY TABLE `t2` (
> +  `a` int(11) NOT NULL,
> +  `new_name` int(11) DEFAULT NULL,
> +  `c` varchar(80) DEFAULT 'X',
> +  `e` enum('a','b','c') DEFAULT NULL,
> +  PRIMARY KEY (`a`)
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1 COMMENT='new comment'
> +drop table t2;
> +create table t1 (a int not null primary key, b int, c varchar(80), e enum('a','b')) engine=aria;
> +insert into t1 (a) values (1),(2),(3);
> +alter online table t1 modify b int default 5;
> +alter online table t1 change b new_name int;
> +alter online table t1 modify e enum('a','b','c');
> +alter online table t1 comment "new comment";
> +show create table t1;
> +Table	Create Table
> +t1	CREATE TABLE `t1` (
> +  `a` int(11) NOT NULL,
> +  `new_name` int(11) DEFAULT NULL,
> +  `c` varchar(80) DEFAULT NULL,
> +  `e` enum('a','b','c') DEFAULT NULL,
> +  PRIMARY KEY (`a`)
> +) ENGINE=Aria DEFAULT CHARSET=latin1 PAGE_CHECKSUM=1 COMMENT='new comment'
> +alter online table t1 page_checksum=1;
> +alter online table t1 page_checksum=0;
> +ERROR 0A000: LOCK=NONE is not supported for this operation. Try LOCK=SHARED

Why LOCK=NONE would be not supported now?
Shouldn't your new feature allow just everything with LOCK=NONE?

> +drop table t1;
> +create table t1 (a int not null primary key, b int, c varchar(80), e enum('a','b'));
> +insert into t1 (a) values (1),(2),(3);
> +alter online table t1 drop column b, add b int;
> +ERROR 0A000: LOCK=NONE is not supported for this operation. Try LOCK=SHARED
...
> diff --git a/mysql-test/main/alter_table_online.combinations b/mysql-test/main/alter_table_online.combinations
> new file mode 100644
> index 00000000000..ae144432c68
> --- /dev/null
> +++ b/mysql-test/main/alter_table_online.combinations
> @@ -0,0 +1,2 @@
> +[innodb]
> +[rocksdb]

what about non-trans tables?

> diff --git a/mysql-test/main/alter_table_online.test b/mysql-test/main/alter_table_online.test
> index 6ef9661c43b..815483d252d 100644
> --- a/mysql-test/main/alter_table_online.test
> +++ b/mysql-test/main/alter_table_online.test
> @@ -1,335 +1,596 @@
> -#
> -# Test of ALTER ONLINE TABLE syntax
> -#
> -
> ---source include/have_innodb.inc
> ---source include/have_partition.inc
> -#
> -# Test of things that can be done online
> -#
> -
> -create table t1 (a int not null primary key, b int, c varchar(80), e enum('a','b')) engine=myisam;
> -insert into t1 (a) values (1),(2),(3);
> -
> -alter online table t1 modify b int default 5, alter c set default 'X';
> -alter online table t1 change b new_name int;
> -alter online table t1 modify e enum('a','b','c');
> -alter online table t1 comment "new comment";
> -alter table t1 add constraint q check (a > 0);
> -alter online table t1 drop constraint q;
> -
> -# No OPs
> -
> -alter online table t1 algorithm=INPLACE, lock=NONE;
> -alter online table t1;
> -alter table t1 algorithm=INPLACE;
> -alter table t1 lock=NONE;
> -show create table t1;
> -drop table t1;
> +-- source include/have_debug_sync.inc
> +-- source include/not_embedded.inc
> +-- source alter_table_online_binlog.inc
>  
> -#
> -# everything with temporary tables is "online", i.e. without locks
> -#
> -create temporary table t1 (a int not null primary key, b int, c varchar(80), e enum('a','b'));
> -insert into t1 (a) values (1),(2),(3);
> -
> -alter online table t1 modify b int default 5, alter c set default 'X';
> -alter online table t1 change b new_name int;
> -alter online table t1 modify e enum('a','b','c');
> -alter online table t1 comment "new comment";
> -alter online table t1 rename to t2;
> -show create table t2;
> -drop table t2;
> +-- disable_query_log
> +-- if ($MTR_COMBINATION_INNODB) {
> +-- source include/have_innodb.inc
> +set default_storage_engine= innodb;
> +-- }
> +-- if ($MTR_COMBINATION_ROCKSDB) {
> +-- source include/have_rocksdb.inc
> +set default_storage_engine= rocksdb;
> +-- }
> +-- enable_query_log
> +
> +-- let $default_engine= `select @@default_storage_engine`
> +
> +--connect (con2, localhost, root,,)
> +--connection default
> +
> +
> +--echo #
> +--echo # Test insert
> +--echo #
> +
> +--echo # Insert and add column
> +create or replace table t1 (a int) engine=innodb;

why do you specify the engine explicitly if you run it in two engine
combinations?

> +insert t1 values (5);
> +
> +--connection con2
> +--send
> +set debug_sync= 'now WAIT_FOR ended';
> +
> +--connection default
> +set debug_sync= 'alter_table_copy_end SIGNAL ended WAIT_FOR end';
> +
...
> +
> +--connection default
> +--reap
> +--sorted_result
> +select * from t1;
> +
> +--echo #
> +--echo # MYISAM. Only Inserts can be tested.

why?

> +--echo #
> +
> +create or replace table t1 (a int) engine=myisam;
> +insert t1 values (5);
> +
> +--connection con2
> +--send
> +set debug_sync= 'now WAIT_FOR ended';
> +
> +--connection default
> +set debug_sync= 'alter_table_copy_end SIGNAL ended WAIT_FOR end';
> +
> +--send
> +alter table t1 add b int NULL, algorithm= copy, lock= none;
> +
...
> diff --git a/mysql-test/main/mdl_sync.test b/mysql-test/main/mdl_sync.test
> index 3df19aca806..8c6bc76f5ca 100644
> --- a/mysql-test/main/mdl_sync.test
> +++ b/mysql-test/main/mdl_sync.test
> @@ -42,7 +42,7 @@ connection con1;
>  set debug_sync='mdl_upgrade_lock SIGNAL parked WAIT_FOR go';
>  --send alter table t1 rename t3
>  
> -connection default;
> +  connection default;

eh? (here and many other changes in this file)

>  --echo connection: default
>  set debug_sync= 'now WAIT_FOR parked';
>  
> diff --git a/mysql-test/suite/gcol/inc/gcol_select.inc b/mysql-test/suite/gcol/inc/gcol_select.inc
> index 2386c55fdbc..8f6314233bd 100644
> --- a/mysql-test/suite/gcol/inc/gcol_select.inc
> +++ b/mysql-test/suite/gcol/inc/gcol_select.inc
> @@ -872,7 +872,9 @@ CREATE TABLE t1(a INT);
>  INSERT INTO t1 VALUES(2147483647);
>  ALTER TABLE t1 ADD COLUMN h INT AS (a) VIRTUAL;
>  ALTER TABLE t1 CHANGE h i INT AS (a) VIRTUAL, ALGORITHM=COPY;
> +--error ER_WARN_DATA_OUT_OF_RANGE,ER_ALTER_OPERATION_NOT_SUPPORTED_REASON

why two? embedded, again?

>  ALTER TABLE t1 ADD COLUMN b SMALLINT AS (a) VIRTUAL, ALGORITHM=COPY, LOCK=NONE;
> +--error ER_WARN_DATA_OUT_OF_RANGE,ER_ALTER_OPERATION_NOT_SUPPORTED_REASON
>  ALTER TABLE t1 ADD COLUMN e SMALLINT AS (a) VIRTUAL, ALGORITHM=COPY, LOCK=NONE;
>  ALTER TABLE t1 ADD COLUMN f SMALLINT AS (a) VIRTUAL, ALGORITHM=COPY, LOCK=SHARED;
>  ALTER TABLE t1 ADD COLUMN g SMALLINT AS (a) VIRTUAL, ALGORITHM=COPY, LOCK=EXCLUSIVE;
> diff --git a/sql/handler.h b/sql/handler.h
> index fe61666bf20..5e46fa59d7e 100644
> --- a/sql/handler.h
> +++ b/sql/handler.h
> @@ -3538,6 +3543,7 @@ class handler :public Sql_alloc
>    /** to be actually called to get 'check()' functionality*/
>    int ha_check(THD *thd, HA_CHECK_OPT *check_opt);
>    int ha_repair(THD* thd, HA_CHECK_OPT* check_opt);
> +  virtual void open_read_view(){}

why did you change to that from start_consistent_snapshot()?

>    void ha_start_bulk_insert(ha_rows rows, uint flags= 0)
>    {
>      DBUG_ENTER("handler::ha_start_bulk_insert");
> diff --git a/sql/log.cc b/sql/log.cc
> index 70ceecc66f8..9beab80773c 100644
> --- a/sql/log.cc
> +++ b/sql/log.cc
> @@ -101,6 +102,9 @@ static int binlog_flush_cache(THD *thd, binlog_cache_mngr *cache_mngr,
>                                Log_event *end_ev, bool all, bool using_stmt,
>                                bool using_trx, bool is_ro_1pc);
>  
> +int binlog_online_alter_commit(THD *thd, bool all);
> +void binlog_online_alter_rollback(THD *thd, bool all);

static?

> +
>  static const LEX_CSTRING write_error_msg=
>      { STRING_WITH_LEN("error writing to the binary log") };
>  
> @@ -3565,6 +3613,91 @@ bool MYSQL_BIN_LOG::open_index_file(const char *index_file_name_arg,
>  }
>  
>  
> +bool Event_log::open(const char *log_name,
> +                     const char *new_name, ulong next_file_number,
> +                     enum cache_type io_cache_type_arg)
> +{
> +  bool error= false;
> +  if (log_name || new_name)
> +  {
> +    error= MYSQL_LOG::open(
> +#ifdef HAVE_PSI_INTERFACE
> +          0,
> +#endif
> +          log_name, LOG_NORMAL, new_name, next_file_number, io_cache_type_arg);
> +  }
> +  else
> +  {
> +#ifdef HAVE_PSI_INTERFACE
> +    /* Keep the key for reopen */
> +    m_log_file_key= 0;
> +#endif
> +    error= init_io_cache(&log_file, -1, LOG_BIN_IO_SIZE,
> +                         io_cache_type_arg, 0, 0,
> +                         MYF(MY_WME | MY_NABP | MY_WAIT_IF_FULL));
> +
> +    log_state= LOG_OPENED;
> +    inited= true;
> +  }
> +  if (error)
> +    return error;
> +
> +  longlong bytes_written= write_description_event(
> +                              (enum_binlog_checksum_alg)binlog_checksum_options,
> +                              encrypt_binlog, false);
> +  error= bytes_written < 0;
> +  return error;
> +}
> +
> +longlong
> +Event_log::write_description_event(enum_binlog_checksum_alg checksum_alg,
> +                                   bool encrypt, bool dont_set_created)
> +{
> +  Format_description_log_event s(BINLOG_VERSION);
> +  /*
> +    don't set LOG_EVENT_BINLOG_IN_USE_F for SEQ_READ_APPEND io_cache
> +    as we won't be able to reset it later
> +  */
> +  if (io_cache_type == WRITE_CACHE)

this was incorrectly rebased. in the previous patch you *moved*
this block of code from MYSQL_BIN_LOG::open into a separate method.

here you copied it. you forgot to delete it from MYSQL_BIN_LOG::open().

> +    s.flags |= LOG_EVENT_BINLOG_IN_USE_F;
> +  s.checksum_alg= checksum_alg;
> +
> +  crypto.scheme = 0;
> +  DBUG_ASSERT(s.checksum_alg != BINLOG_CHECKSUM_ALG_UNDEF);
> +  if (!s.is_valid())
> +    return -1;
> +  s.dont_set_created= dont_set_created;
> +  if (write_event(&s, 0, &log_file))
> +    return -1;
> +
> +  if (encrypt)
> +  {
> +    uint key_version= encryption_key_get_latest_version(ENCRYPTION_KEY_SYSTEM_DATA);
> +    if (key_version == ENCRYPTION_KEY_VERSION_INVALID)
> +    {
> +      sql_print_error("Failed to enable encryption of binary logs");
> +      return -1;
> +    }
> +
> +    if (key_version != ENCRYPTION_KEY_NOT_ENCRYPTED)
> +    {
> +      if (my_random_bytes(crypto.nonce, sizeof(crypto.nonce)))
> +        return -1;
> +
> +      Start_encryption_log_event sele(1, key_version, crypto.nonce);
> +      sele.checksum_alg= s.checksum_alg;
> +      if (write_event(&sele, 0, &log_file))
> +        return -1;
> +
> +      // Start_encryption_log_event is written, enable the encryption
> +      if (crypto.init(sele.crypto_scheme, key_version))
> +        return -1;
> +    }
> +  }
> +  return (longlong)s.data_written;
> +}
> +
> +
>  /**
>    Open a (new) binlog file.
>  
> @@ -7252,6 +7477,160 @@ class CacheWriter: public Log_event_writer
>    bool first;
>  };
>  
> +int cache_copy(IO_CACHE *to, IO_CACHE *from)
> +{
> +  DBUG_ENTER("cache_copy");
> +  if (reinit_io_cache(from, READ_CACHE, 0, 0, 0))
> +    DBUG_RETURN(ER_ERROR_ON_WRITE);
> +  size_t bytes_in_cache= my_b_bytes_in_cache(from);
> +
> +  do
> +  {
> +    my_b_write(to, from->read_pos, bytes_in_cache);
> +
> +    from->read_pos += bytes_in_cache;
> +    bytes_in_cache= my_b_fill(from);
> +    if (from->error || to->error)
> +      DBUG_RETURN(ER_ERROR_ON_WRITE);
> +  } while (bytes_in_cache);
> +
> +  DBUG_RETURN(0);
> +}
> +
> +int binlog_online_alter_commit(THD *thd, bool all)
> +{
> +  DBUG_ENTER("online_alter_commit");
> +  int error= 0;
> +#ifdef HAVE_REPLICATION
> +
> +  if (thd->online_alter_cache_list.empty())
> +    DBUG_RETURN(0);
> +
> +  bool is_ending_transaction= ending_trans(thd, all);
> +
> +  for (auto &cache_mngr: thd->online_alter_cache_list)
> +  {
> +    auto *binlog= cache_mngr.share->online_alter_binlog;
> +    DBUG_ASSERT(binlog);
> +
> +    error= binlog_flush_pending_rows_event(thd,
> +                                           /*
> +                                             do not set STMT_END for last event
> +                                             to leave table open in altering thd
> +                                           */
> +                                           false,
> +                                           true,
> +                                           binlog,
> +                                           is_ending_transaction
> +                                           ? &cache_mngr.trx_cache
> +                                           : &cache_mngr.stmt_cache);
> +    if (is_ending_transaction)
> +    {
> +      mysql_mutex_lock(binlog->get_log_lock());
> +      error= binlog->write_cache(thd, &cache_mngr.trx_cache.cache_log);
> +
> +      mysql_mutex_unlock(binlog->get_log_lock());
> +    }
> +    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.

> +    }
> +
> +    if (error)
> +    {
> +      my_error(ER_ERROR_ON_WRITE, MYF(ME_ERROR_LOG),
> +               binlog->get_name(), errno);
> +      binlog_online_alter_cleanup(thd->online_alter_cache_list,
> +                                  is_ending_transaction);
> +      DBUG_RETURN(error);
> +    }
> +  }
> +
> +  binlog_online_alter_cleanup(thd->online_alter_cache_list,
> +                              is_ending_transaction);
> +
> +  for (TABLE *table= thd->open_tables; table; table= table->next)
> +  {
> +    table->online_alter_cache= NULL;

Why?

> +  }
> +#endif // HAVE_REPLICATION
> +  DBUG_RETURN(error);
> +}
> +
> +void binlog_online_alter_rollback(THD *thd, bool all)
> +{
> +#ifdef HAVE_REPLICATION
> +  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);
> +#endif // HAVE_REPLICATION
> +}
> +
> +SAVEPOINT** find_savepoint_in_list(THD *thd, LEX_CSTRING name,
> +                                   SAVEPOINT ** const list);
> +
> +SAVEPOINT* savepoint_add(THD *thd, LEX_CSTRING name, SAVEPOINT **list,
> +                         int (*release_old)(THD*, SAVEPOINT*));

may be in transaction.h?

> +
> +int online_alter_savepoint_set(THD *thd, LEX_CSTRING name)
> +{
> +
> +  DBUG_ENTER("binlog_online_alter_savepoint");
> +#ifdef HAVE_REPLICATION
> +  if (thd->online_alter_cache_list.empty())
> +    DBUG_RETURN(0);
> +
> +  if (savepoint_alloc_size < sizeof (SAVEPOINT) + sizeof(my_off_t))
> +    savepoint_alloc_size= sizeof (SAVEPOINT) + sizeof(my_off_t);
> +
> +  for (auto &cache: thd->online_alter_cache_list)
> +  {
> +    if (cache.share->db_type()->savepoint_set == NULL)
> +      continue;
> +
> +    SAVEPOINT *sv= savepoint_add(thd, name, &cache.sv_list, NULL);

wouldn't it be simpler to log SAVEPOINT and ROLLBACK/RELEASE
as Query_log_event to the cache? Then you won't need to do
any maintenance.

> +    if(unlikely(sv == NULL))
> +      DBUG_RETURN(1);
> +    my_off_t *pos= (my_off_t*)(sv+1);
> +    *pos= cache.trx_cache.get_byte_position();
> +
> +    sv->prev= cache.sv_list;
> +    cache.sv_list= sv;
> +  }
> +#endif
> +  DBUG_RETURN(0);
> +}
> +
> +int online_alter_savepoint_rollback(THD *thd, LEX_CSTRING name)
> +{
> +  DBUG_ENTER("online_alter_savepoint_rollback");
> +#ifdef HAVE_REPLICATION
> +  for (auto &cache: thd->online_alter_cache_list)
> +  {
> +    if (cache.share->db_type()->savepoint_set == NULL)
> +      continue;
> +
> +    SAVEPOINT **sv= find_savepoint_in_list(thd, name, &cache.sv_list);
> +    // sv is null if savepoint was set up before online table was modified
> +    my_off_t pos= *sv ? *(my_off_t*)(*sv+1) : 0;
> +
> +    cache.trx_cache.restore_savepoint(pos);
> +  }
> +
> +#endif
> +  DBUG_RETURN(0);
> +}
> +
>  /*
>    Write the contents of a cache to the binary log.
>  
> diff --git a/sql/rpl_rli.h b/sql/rpl_rli.h
> index cc807852bf2..57757ecf3dd 100644
> --- a/sql/rpl_rli.h
> +++ b/sql/rpl_rli.h
> @@ -900,14 +900,20 @@ struct rpl_group_info
>      }
>    }
>  
> -  bool get_table_data(TABLE *table_arg, table_def **tabledef_var, TABLE **conv_table_var) const
> +  bool get_table_data(TABLE *table_arg, table_def **tabledef_var,
> +          TABLE **conv_table_var,
> +          const Copy_field *copy[], const Copy_field **copy_end) const
>    {
>      DBUG_ASSERT(tabledef_var && conv_table_var);
>      for (TABLE_LIST *ptr= tables_to_lock ; ptr != NULL ; ptr= ptr->next_global)
>        if (ptr->table == table_arg)
>        {
> -        *tabledef_var= &static_cast<RPL_TABLE_LIST*>(ptr)->m_tabledef;
> -        *conv_table_var= static_cast<RPL_TABLE_LIST*>(ptr)->m_conv_table;
> +        auto *rpl_table_list= static_cast<RPL_TABLE_LIST*>(ptr);
> +        if (rpl_table_list->m_tabledef_valid)

when can it be false?

> +          *tabledef_var= &rpl_table_list->m_tabledef;
> +        *conv_table_var= rpl_table_list->m_conv_table;
> +        *copy= rpl_table_list->m_online_alter_copy_fields;
> +        *copy_end= rpl_table_list->m_online_alter_copy_fields_end;
>          DBUG_PRINT("debug", ("Fetching table data for table %s.%s:"
>                               " tabledef: %p, conv_table: %p",
>                               table_arg->s->db.str, table_arg->s->table_name.str,
> diff --git a/sql/sql_table.cc b/sql/sql_table.cc
> index b07efb29bba..9dfe2178667 100644
> --- a/sql/sql_table.cc
> +++ b/sql/sql_table.cc
> @@ -9533,6 +9539,19 @@ bool mysql_alter_table(THD *thd, const LEX_CSTRING *new_db,
>      has been already processed.
>    */
>    table_list->required_type= TABLE_TYPE_NORMAL;
> +  
> +  
> +  if (alter_info->requested_lock == Alter_info::ALTER_TABLE_LOCK_SHARED
> +      || alter_info->requested_lock == Alter_info::ALTER_TABLE_LOCK_EXCLUSIVE
> +      || thd->locked_tables_mode == LTM_LOCK_TABLES
> +      || thd->lex->sql_command == SQLCOM_OPTIMIZE
> +      || alter_info->algorithm(thd) == Alter_info::ALTER_TABLE_ALGORITHM_NOCOPY)

why only NOCOPY? What about INPLACE and INSTANT?

the rest of the condition looks fine.

> +    online= false;
> +
> +  if (online)
> +  {
> +    table_list->lock_type= TL_READ;
> +  }
>  
>    DEBUG_SYNC(thd, "alter_table_before_open_tables");
>  
> @@ -10961,6 +10983,58 @@ bool mysql_trans_commit_alter_copy_data(THD *thd)
>    DBUG_RETURN(error);
>  }
>  
> +#ifdef HAVE_REPLICATION
> +static int online_alter_read_from_binlog(THD *thd, rpl_group_info *rgi,
> +                                         Cache_flip_event_log *log)
> +{
> +  MEM_ROOT event_mem_root;
> +  Query_arena backup_arena;
> +  Query_arena event_arena(&event_mem_root, Query_arena::STMT_INITIALIZED);
> +  init_sql_alloc(key_memory_gdl, &event_mem_root,
> +                 MEM_ROOT_BLOCK_SIZE, 0, MYF(0));
> +
> +  int error= 0;
> +
> +  IO_CACHE *log_file= log->flip();
> +
> +  thd_progress_report(thd, 0, my_b_write_tell(log_file));
> +
> +  Abort_on_warning_instant_set old_abort_on_warning(thd, 0);
> +  do
> +  {
> +    const auto *descr_event= rgi->rli->relay_log.description_event_for_exec;
> +    auto *ev= Log_event::read_log_event(log_file, descr_event, false);
> +    if (!ev)
> +      break;
> +
> +    ev->thd= thd;
> +    thd->set_n_backup_active_arena(&event_arena, &backup_arena);
> +    error= ev->apply_event(rgi);
> +    thd->restore_active_arena(&event_arena, &backup_arena);
> +
> +    event_arena.free_items();
> +    free_root(&event_mem_root, MYF(MY_KEEP_PREALLOC));
> +    if (ev != rgi->rli->relay_log.description_event_for_exec)
> +      delete ev;
> +    thd_progress_report(thd, my_b_tell(log_file), thd->progress.max_counter);
> +    DEBUG_SYNC(thd, "alter_table_online_progress");
> +  } while(!error);
> +
> +  return error;
> +}
> +#endif // HAVE_REPLICATION
> +
> +static void online_alter_cleanup_binlog(THD *thd, TABLE_SHARE *s)
> +{
> +#ifdef HAVE_REPLICATION
> +  if (!s->online_alter_binlog)
> +    return;
> +  // s->online_alter_binlog->reset_logs(thd, false, NULL, 0, 0);

forgot to delete?

> +  s->online_alter_binlog->cleanup();
> +  s->online_alter_binlog->~Cache_flip_event_log();
> +  s->online_alter_binlog= NULL;
> +#endif
> +}
>  
>  static int
>  copy_data_between_tables(THD *thd, TABLE *from, TABLE *to,
> @@ -11306,6 +11431,76 @@ copy_data_between_tables(THD *thd, TABLE *from, TABLE *to,
>    cleanup_done= 1;
>    to->file->extra(HA_EXTRA_NO_IGNORE_DUP_KEY);
>  
> +#ifdef HAVE_REPLICATION
> +  if (likely(online && error < 0))
> +  {
> +    Ha_trx_info *trx_info_save= thd->transaction->all.ha_list;
> +    thd->transaction->all.ha_list = NULL;

why?

> +    thd_progress_next_stage(thd);
> +    Table_map_log_event table_event(thd, from, from->s->table_map_id,
> +                                    from->file->has_transactions());
> +    Relay_log_info rli(false);
> +    rpl_group_info rgi(&rli);
> +    RPL_TABLE_LIST rpl_table(to, TL_WRITE, from, table_event.get_table_def(),
> +                             copy, copy_end);
> +    Cache_flip_event_log *binlog= from->s->online_alter_binlog;
> +    rgi.thd= thd;
> +    rgi.tables_to_lock= &rpl_table;
> +
> +    rgi.m_table_map.set_table(from->s->table_map_id, to);
> +
> +    DBUG_ASSERT(binlog->is_open());
> +
> +    rli.relay_log.description_event_for_exec=
> +                                            new Format_description_log_event(4);
> +
> +    // We restore bitmaps, because update event is going to mess up with them.
> +    to->default_column_bitmaps();
> +
> +    error= online_alter_read_from_binlog(thd, &rgi, binlog);
> +
> +    DEBUG_SYNC(thd, "alter_table_online_before_lock");
> +
> +    int lock_error=
> +        thd->mdl_context.upgrade_shared_lock(from->mdl_ticket, MDL_EXCLUSIVE,
> +                                     (double)thd->variables.lock_wait_timeout);
> +    if (!error)
> +      error= lock_error;
> +
> +    if (!error)
> +    {
> +      thd_progress_next_stage(thd);
> +      error= online_alter_read_from_binlog(thd, &rgi, binlog);
> +    }
> +
> +    thd->transaction->all.ha_list = trx_info_save;
> +  }
> +  else if (unlikely(online)) // error was on copy stage
> +  {
> +    /*
> +       We need to issue a barrier to clean up gracefully.
> +       Without this, following possible:
> +       T1: ALTER TABLE starts
> +       T2: INSERT starts
> +       T1: ALTER TABLE fails with error (i.e. ER_DUP_KEY)
> +       T1: from->s->online_alter_binlog sets to NULL
> +       T2: INSERT committs
> +       T2: thd->online_alter_cache_list is not empty
> +       T2: binlog_commit: DBUG_ASSERT(binlog); is issued.

1. do you have a test for that?
2. can it be fixed, like, on iterating thd->online_alter_cache_list
   thd notices that from->s->online_alter_cache_list is NULL, so
   it simply discards the cache? Then you won't need MDL_SHARED_NO_WRITE

> +    */
> +    // Ignore the return result. We already have an error.
> +    thd->mdl_context.upgrade_shared_lock(from->mdl_ticket, 
> +                                         MDL_SHARED_NO_WRITE,
> +                                         thd->variables.lock_wait_timeout);
> +  }
> +#endif
> +
> +  if (error > 0 && !from->s->tmp_table)
> +  {
> +    /* We are going to drop the temporary table */
> +    to->file->extra(HA_EXTRA_PREPARE_FOR_DROP);
> +  }
> +
>    DEBUG_SYNC(thd, "copy_data_between_tables_before_reset_backup_lock");
>    if (backup_reset_alter_copy_lock(thd))
>      error= 1;

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx