maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #13086
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
