- developers - lists.mariadb.org

Re: [Commits] cb06612a9da: MDEV-31655: Parallel replication deadlock victim preference code errorneously removed
by Kristian Nielsen 15 Aug '23

15 Aug '23

Hi Marko, I'd like to ask you to review the below patch for MDEV-31655. https://jira.mariadb.org/browse/MDEV-31655 https://github.com/MariaDB/server/commit/cb06612a9da09a7981ada84768793f2ff3… This patch restores code in InnoDB that was originally part of the parallel replication implementation. It makes InnoDB choose the right victim when two transactions in parallel replication deadlock with each other. The code was erroneously removed as part of the VATS work in InnoDB. I saw that you later deprecated and eventually removed the VATS code. I saw that the InnoDB deadlock code is substantially changed in 10.6, so I will need to rework the patch in that version. But I wanted to ask your opinion about this 10.4 version first. In the patch, note in particular the modification to has_higher_priority(), which I think has the effect of choosing which of two waiting transactions to grant a lock to. I don't remember adding this myself, so it might have been added as part of VATS work. But it sounds appropriate; if T1 and T2 are both waiting for a lock, there is no point in granting it to T2 over T1, as then T2 will just be immediately killed and rolled back by replication. - Kristian. Kristian Nielsen via commits <commits(a)lists.mariadb.org> writes: > revision-id: cb06612a9da09a7981ada84768793f2ff3ef842c (mariadb-10.4.30-3-gcb06612a9da) > parent(s): dbe5c20b755b87f67d87990c3baabc866667e41b > author: Kristian Nielsen > committer: Kristian Nielsen > timestamp: 2023-07-11 00:31:29 +0200 > message: > > MDEV-31655: Parallel replication deadlock victim preference code errorneously removed > > Restore code to make InnoDB choose the second transaction as a deadlock > victim if two transactions deadlock that need to commit in-order for > parallel replication. This code was erroneously removed when VATS was > implemented in InnoDB. > > Also add a test case for InnoDB choosing the right deadlock victim. > > Signed-off-by: Kristian Nielsen <knielsen(a)knielsen-hq.org> > > --- > .../suite/binlog_encryption/rpl_parallel.result | 42 ++++++++++++- > mysql-test/suite/rpl/r/rpl_parallel.result | 42 ++++++++++++- > mysql-test/suite/rpl/t/rpl_parallel.test | 71 +++++++++++++++++++++- > sql/rpl_parallel.cc | 7 ++- > sql/sql_class.cc | 43 +++++++++++++ > storage/innobase/lock/lock0lock.cc | 12 ++++ > storage/innobase/trx/trx0trx.cc | 12 ++++ > 7 files changed, 225 insertions(+), 4 deletions(-) > > diff --git a/mysql-test/suite/binlog_encryption/rpl_parallel.result b/mysql-test/suite/binlog_encryption/rpl_parallel.result > index b75a66a634a..b24ff7ba53d 100644 > --- a/mysql-test/suite/binlog_encryption/rpl_parallel.result > +++ b/mysql-test/suite/binlog_encryption/rpl_parallel.result > @@ -2,6 +2,7 @@ include/master-slave.inc > [connection master] > connection server_2; > SET @old_parallel_threads=@@GLOBAL.slave_parallel_threads; > +SET @old_parallel_mode=@@GLOBAL.slave_parallel_mode; > SET GLOBAL slave_parallel_threads=10; > ERROR HY000: This operation cannot be performed as you have a running slave ''; run STOP SLAVE '' first > include/stop_slave.inc > @@ -1680,13 +1681,52 @@ a > 2000 > SELECT * FROM t2 WHERE a>=2000 ORDER BY a; > a > +MDEV-31655: Parallel replication deadlock victim preference code erroneously removed > +connection server_1; > +CREATE TABLE t7 (a INT PRIMARY KEY, b INT) ENGINE=InnoDB; > +BEGIN; > +COMMIT; > +include/save_master_gtid.inc > +connection server_2; > +include/sync_with_master_gtid.inc > +include/stop_slave.inc > +set @@global.slave_parallel_threads= 5; > +set @@global.slave_parallel_mode= conservative; > +SET @old_dbug= @@GLOBAL.debug_dbug; > +SET GLOBAL debug_dbug= "+d,rpl_mdev31655_zero_retries"; > +connection master; > +SET @old_dbug= @@SESSION.debug_dbug; > +SET SESSION debug_dbug="+d,binlog_force_commit_id"; > +SET @commit_id= 1+1000; > +SET @commit_id= 2+1000; > +SET @commit_id= 3+1000; > +SET @commit_id= 4+1000; > +SET @commit_id= 5+1000; > +SET @commit_id= 6+1000; > +SET @commit_id= 7+1000; > +SET @commit_id= 8+1000; > +SET @commit_id= 9+1000; > +SET @commit_id= 10+1000; > +SET SESSION debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > +COUNT(*) SUM(a*100*b) > +10 225000 > +include/save_master_gtid.inc > +connection server_2; > +include/start_slave.inc > +include/sync_with_master_gtid.inc > +SET GLOBAL debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > +COUNT(*) SUM(a*100*b) > +10 225000 > connection server_2; > include/stop_slave.inc > SET GLOBAL slave_parallel_threads=@old_parallel_threads; > +SET GLOBAL slave_parallel_mode=@old_parallel_mode; > include/start_slave.inc > SET DEBUG_SYNC= 'RESET'; > connection server_1; > DROP function foo; > -DROP TABLE t1,t2,t3,t4,t5,t6; > +DROP TABLE t1,t2,t3,t4,t5,t6,t7; > SET DEBUG_SYNC= 'RESET'; > include/rpl_end.inc > diff --git a/mysql-test/suite/rpl/r/rpl_parallel.result b/mysql-test/suite/rpl/r/rpl_parallel.result > index 9b2e68d366e..ef89d954faa 100644 > --- a/mysql-test/suite/rpl/r/rpl_parallel.result > +++ b/mysql-test/suite/rpl/r/rpl_parallel.result > @@ -2,6 +2,7 @@ include/master-slave.inc > [connection master] > connection server_2; > SET @old_parallel_threads=@@GLOBAL.slave_parallel_threads; > +SET @old_parallel_mode=@@GLOBAL.slave_parallel_mode; > SET GLOBAL slave_parallel_threads=10; > ERROR HY000: This operation cannot be performed as you have a running slave ''; run STOP SLAVE '' first > include/stop_slave.inc > @@ -1679,13 +1680,52 @@ a > 2000 > SELECT * FROM t2 WHERE a>=2000 ORDER BY a; > a > +MDEV-31655: Parallel replication deadlock victim preference code erroneously removed > +connection server_1; > +CREATE TABLE t7 (a INT PRIMARY KEY, b INT) ENGINE=InnoDB; > +BEGIN; > +COMMIT; > +include/save_master_gtid.inc > +connection server_2; > +include/sync_with_master_gtid.inc > +include/stop_slave.inc > +set @@global.slave_parallel_threads= 5; > +set @@global.slave_parallel_mode= conservative; > +SET @old_dbug= @@GLOBAL.debug_dbug; > +SET GLOBAL debug_dbug= "+d,rpl_mdev31655_zero_retries"; > +connection master; > +SET @old_dbug= @@SESSION.debug_dbug; > +SET SESSION debug_dbug="+d,binlog_force_commit_id"; > +SET @commit_id= 1+1000; > +SET @commit_id= 2+1000; > +SET @commit_id= 3+1000; > +SET @commit_id= 4+1000; > +SET @commit_id= 5+1000; > +SET @commit_id= 6+1000; > +SET @commit_id= 7+1000; > +SET @commit_id= 8+1000; > +SET @commit_id= 9+1000; > +SET @commit_id= 10+1000; > +SET SESSION debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > +COUNT(*) SUM(a*100*b) > +10 225000 > +include/save_master_gtid.inc > +connection server_2; > +include/start_slave.inc > +include/sync_with_master_gtid.inc > +SET GLOBAL debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > +COUNT(*) SUM(a*100*b) > +10 225000 > connection server_2; > include/stop_slave.inc > SET GLOBAL slave_parallel_threads=@old_parallel_threads; > +SET GLOBAL slave_parallel_mode=@old_parallel_mode; > include/start_slave.inc > SET DEBUG_SYNC= 'RESET'; > connection server_1; > DROP function foo; > -DROP TABLE t1,t2,t3,t4,t5,t6; > +DROP TABLE t1,t2,t3,t4,t5,t6,t7; > SET DEBUG_SYNC= 'RESET'; > include/rpl_end.inc > diff --git a/mysql-test/suite/rpl/t/rpl_parallel.test b/mysql-test/suite/rpl/t/rpl_parallel.test > index 9ba7a30f2eb..d43cec4df34 100644 > --- a/mysql-test/suite/rpl/t/rpl_parallel.test > +++ b/mysql-test/suite/rpl/t/rpl_parallel.test > @@ -13,6 +13,7 @@ > > --connection server_2 > SET @old_parallel_threads=@@GLOBAL.slave_parallel_threads; > +SET @old_parallel_mode=@@GLOBAL.slave_parallel_mode; > --error ER_SLAVE_MUST_STOP > SET GLOBAL slave_parallel_threads=10; > --source include/stop_slave.inc > @@ -2203,16 +2204,84 @@ SELECT * FROM t1 WHERE a>=2000 ORDER BY a; > SELECT * FROM t2 WHERE a>=2000 ORDER BY a; > > > +--echo MDEV-31655: Parallel replication deadlock victim preference code erroneously removed > +# The problem was that InnoDB would choose the wrong deadlock victim. > +# Create a lot of transactions that can cause deadlocks, and use error > +# injection to check that the first transactions in each group is never > +# selected as deadlock victim. > +--let $rows= 10 > +--let $transactions= 5 > +--let $gcos= 10 > + > +--connection server_1 > +CREATE TABLE t7 (a INT PRIMARY KEY, b INT) ENGINE=InnoDB; > +BEGIN; > +--disable_query_log > +--let $i= 0 > +while ($i < 10) { > + eval INSERT INTO t7 VALUES ($i, 0); > + inc $i; > +} > +--enable_query_log > +COMMIT; > +--source include/save_master_gtid.inc > + > +--connection server_2 > +--source include/sync_with_master_gtid.inc > +--source include/stop_slave.inc > +eval set @@global.slave_parallel_threads= $transactions; > +set @@global.slave_parallel_mode= conservative; > +SET @old_dbug= @@GLOBAL.debug_dbug; > +# This error injection will allow no retries for GTIDs divisible by 1000. > +SET GLOBAL debug_dbug= "+d,rpl_mdev31655_zero_retries"; > + > +--connection master > +SET @old_dbug= @@SESSION.debug_dbug; > +SET SESSION debug_dbug="+d,binlog_force_commit_id"; > + > +--let $j= 1 > +while ($j <= $gcos) { > + eval SET @commit_id= $j+1000; > + --let $i= 0 > + while ($i < $transactions) { > + --disable_query_log > + eval SET SESSION gtid_seq_no= 1000 + 1000*$j + $i; > + BEGIN; > + --let $k= 0 > + while ($k < $rows) { > + eval UPDATE t7 SET b=b+1 WHERE a=(($i+$k) MOD $rows); > + inc $k; > + } > + COMMIT; > + --enable_query_log > + inc $i; > + } > + inc $j; > +} > + > +SET SESSION debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > + > +--source include/save_master_gtid.inc > + > +--connection server_2 > +--source include/start_slave.inc > +--source include/sync_with_master_gtid.inc > +SET GLOBAL debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > + > + > # Clean up. > --connection server_2 > --source include/stop_slave.inc > SET GLOBAL slave_parallel_threads=@old_parallel_threads; > +SET GLOBAL slave_parallel_mode=@old_parallel_mode; > --source include/start_slave.inc > SET DEBUG_SYNC= 'RESET'; > > --connection server_1 > DROP function foo; > -DROP TABLE t1,t2,t3,t4,t5,t6; > +DROP TABLE t1,t2,t3,t4,t5,t6,t7; > SET DEBUG_SYNC= 'RESET'; > > --source include/rpl_end.inc > diff --git a/sql/rpl_parallel.cc b/sql/rpl_parallel.cc > index b550315d69f..1aeb1257c4a 100644 > --- a/sql/rpl_parallel.cc > +++ b/sql/rpl_parallel.cc > @@ -1385,8 +1385,13 @@ handle_rpl_parallel_thread(void *arg) > err= dbug_simulate_tmp_error(rgi, thd);); > if (unlikely(err)) > { > + ulong max_retries= slave_trans_retries; > convert_kill_to_deadlock_error(rgi); > - if (has_temporary_error(thd) && slave_trans_retries > 0) > + DBUG_EXECUTE_IF("rpl_mdev31655_zero_retries", > + if ((rgi->current_gtid.seq_no % 1000) == 0) > + max_retries= 0; > + ); > + if (has_temporary_error(thd) && max_retries > 0) > err= retry_event_group(rgi, rpt, qev); > } > } > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 8ed3f8a9c5e..e6ed7ca1cc4 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -5247,6 +5247,49 @@ thd_need_ordering_with(const MYSQL_THD thd, const MYSQL_THD other_thd) > return 0; > } > > + > +/* > + If the storage engine detects a deadlock, and needs to choose a victim > + transaction to roll back, it can call this function to ask the upper > + server layer for which of two possible transactions is prefered to be > + aborted and rolled back. > + > + In parallel replication, if two transactions are running in parallel and > + one is fixed to commit before the other, then the one that commits later > + will be prefered as the victim - chosing the early transaction as a victim > + will not resolve the deadlock anyway, as the later transaction still needs > + to wait for the earlier to commit. > + > + The return value is -1 if the first transaction is prefered as a deadlock > + victim, 1 if the second transaction is prefered, or 0 for no preference (in > + which case the storage engine can make the choice as it prefers). > +*/ > +extern "C" int > +thd_deadlock_victim_preference(const MYSQL_THD thd1, const MYSQL_THD thd2) > +{ > + rpl_group_info *rgi1, *rgi2; > + > + if (!thd1 || !thd2) > + return 0; > + > + /* > + If the transactions are participating in the same replication domain in > + parallel replication, then request to select the one that will commit > + later (in the fixed commit order from the master) as the deadlock victim. > + */ > + rgi1= thd1->rgi_slave; > + rgi2= thd2->rgi_slave; > + if (rgi1 && rgi2 && > + rgi1->is_parallel_exec && > + rgi1->rli == rgi2->rli && > + rgi1->current_gtid.domain_id == rgi2->current_gtid.domain_id) > + return rgi1->gtid_sub_id < rgi2->gtid_sub_id ? 1 : -1; > + > + /* No preferences, let the storage engine decide. */ > + return 0; > +} > + > + > extern "C" int thd_non_transactional_update(const MYSQL_THD thd) > { > return(thd->transaction.all.modified_non_trans_table); > diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc > index b1cf2152cd6..469d03eaa06 100644 > --- a/storage/innobase/lock/lock0lock.cc > +++ b/storage/innobase/lock/lock0lock.cc > @@ -49,6 +49,8 @@ Created 5/7/1996 Heikki Tuuri > #include <mysql/service_wsrep.h> > #endif /* WITH_WSREP */ > > +extern "C" int thd_deadlock_victim_preference(const MYSQL_THD thd1, const MYSQL_THD thd2); > + > /** Lock scheduling algorithm */ > ulong innodb_lock_schedule_algorithm; > > @@ -1538,6 +1540,16 @@ static bool has_higher_priority(lock_t *lock1, lock_t *lock2) > } else if (!lock_get_wait(lock2)) { > return false; > } > + // Ask the upper server layer if any of the two trx should be prefered. > + int preference = thd_deadlock_victim_preference(lock1->trx->mysql_thd, > + lock2->trx->mysql_thd); > + if (preference == -1) { > + // lock1 is preferred as a victim, so lock2 has higher priority > + return false; > + } else if (preference == 1) { > + // lock2 is preferred as a victim, so lock1 has higher priority > + return true; > + } > return lock1->trx->start_time_micro <= lock2->trx->start_time_micro; > } > > diff --git a/storage/innobase/trx/trx0trx.cc b/storage/innobase/trx/trx0trx.cc > index 7cd95878b0c..0771d764fb6 100644 > --- a/storage/innobase/trx/trx0trx.cc > +++ b/storage/innobase/trx/trx0trx.cc > @@ -52,6 +52,9 @@ Created 3/26/1996 Heikki Tuuri > #include <set> > #include <new> > > +extern "C" > +int thd_deadlock_victim_preference(const MYSQL_THD thd1, const MYSQL_THD thd2); > + > /** The bit pattern corresponding to TRX_ID_MAX */ > const byte trx_id_max_bytes[8] = { > 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff > @@ -1906,6 +1909,15 @@ trx_weight_ge( > { > ibool a_notrans_edit; > ibool b_notrans_edit; > + int pref; > + > + /* First ask the upper server layer if it has any preference for which > + to prefer as a deadlock victim. */ > + pref= thd_deadlock_victim_preference(a->mysql_thd, b->mysql_thd); > + if (pref < 0) > + return FALSE; > + else if (pref > 0) > + return TRUE; > > /* If mysql_thd is NULL for a transaction we assume that it has > not edited non-transactional tables. */ > _______________________________________________ > commits mailing list -- commits(a)lists.mariadb.org > To unsubscribe send an email to commits-leave(a)lists.mariadb.org

2 7

Re: 9c52eede2c8: MDEV-31646 Online alter applies binlog cache limit to cache writes
by Sergei Golubchik 15 Aug '23

15 Aug '23

Hi, Nikita, On Aug 14, Nikita Malyavin wrote: > revision-id: 9c52eede2c8 (mariadb-11.0.1-175-g9c52eede2c8) > parent(s): 209c894b431 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-08-11 16:29:37 +0400 > message: > > MDEV-31646 Online alter applies binlog cache limit to cache writes > > 1. Make online disk writes unlimited, same as filesort does. > 2. Make proper error handling -- in 32-bit build IO_CACHE capacity limit is > 4GB, so it is quite possible to overfill there. > 3. Event_log::write_cache complicated with event reparsing, and as it was > proven by QA, contains some mistakes. Rewrite introbuce a simpler and much > faster version, not featuring reparsing and therefore copying a whole > buffer at once. This also disables checksums and crypto. > > As a result, online alter is untied of several binlog variables, which was > a second aim of this patch. > > diff --git a/mysql-test/main/alter_table_online_debug.result b/mysql-test/main/alter_table_online_debug.result > index 021267a15f5..3a615372ed5 100644 > --- a/mysql-test/main/alter_table_online_debug.result > +++ b/mysql-test/main/alter_table_online_debug.result > @@ -409,23 +409,23 @@ set debug_sync= 'now SIGNAL start_replication'; > set debug_sync= 'now WAIT_FOR applied'; > select stage, progress from INFORMATION_SCHEMA.PROCESSLIST where id = @con; > stage progress > -3 51.220 > +3 53.390 why is that? > set debug_sync= 'now SIGNAL proceed WAIT_FOR applied'; > select stage, progress from INFORMATION_SCHEMA.PROCESSLIST where id = @con; > stage progress > diff --git a/sql/log.cc b/sql/log.cc > index d9e81700463..8c57487d57e 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -2286,13 +2286,21 @@ int binlog_log_row_online_alter(TABLE* table, const uchar *before_record, > MY_BITMAP *old_rpl_write_set= table->rpl_write_set; > table->rpl_write_set= &table->s->all_set; > > + table->online_alter_cache->store_prev_position(); > int error= (*log_func)(thd, table, table->s->online_alter_binlog, > - table->online_alter_cache, true, > + table->online_alter_cache, > + table->file->has_transactions_and_rollback(), > before_record, after_record); > > table->rpl_write_set= old_rpl_write_set; > > - return unlikely(error) ? HA_ERR_RBR_LOGGING_FAILED : 0; > + if (unlikely(error)) > + { > + table->online_alter_cache->restore_prev_position(); what is this for? > + return HA_ERR_RBR_LOGGING_FAILED; > + } > + > + return 0; > } > > static void > @@ -7748,7 +7763,7 @@ static int binlog_online_alter_end_trans(THD *thd, bool all, bool commit) > { > DBUG_ASSERT(cache.cache_log.type != READ_CACHE); > mysql_mutex_lock(binlog->get_log_lock()); > - error= binlog->write_cache(thd, &cache.cache_log); > + error= binlog->write_cache_raw(thd, &cache.cache_log); > mysql_mutex_unlock(binlog->get_log_lock()); > } > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: ca64ddcc709: MDEV-31646 Online alter applies binlog cache limit to cache writes
by Sergei Golubchik 10 Aug '23

10 Aug '23

Hi, Nikita, This is a review of all the three commits On Jul 20, Nikita Malyavin wrote: > revision-id: ca64ddcc709 (mariadb-11.0.1-144-gca64ddcc709) > parent(s): f34c7419cb8 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-07-19 02:31:32 +0400 > message: > > MDEV-31646 Online alter applies binlog cache limit to cache writes > diff --git a/sql/log.cc b/sql/log.cc > index afd0643fe82..7466d63d325 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -2291,8 +2294,14 @@ int binlog_log_row_online_alter(TABLE* table, const uchar *before_record, > before_record, after_record); > > table->rpl_write_set= old_rpl_write_set; > + thd->pop_internal_handler(); > + > + if (unlikely(error)) > + push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE, ER_DISK_FULL, > + "Broken online alter log. " > + "ALTER TABLE will finish with error."); I don't think so. Why would a DML statement get a warning about the concurrently running ALTER TABLE? It didn't do anything wrong, I don't think there should be a warning here Like, someone is running a series of DML statements, or the same statement many times. At some point another connection runs ALTER TABLE and suddenly those perfectly good DML statements start spewing out warnings. ALTER ends and all is clear again - looks wrong to me > > - return unlikely(error) ? HA_ERR_RBR_LOGGING_FAILED : 0; > + return 0; > } > > static void > @@ -2592,7 +2601,7 @@ void Event_log::set_write_error(THD *thd, bool is_transactional) > DBUG_VOID_RETURN; > } > > -bool Event_log::check_write_error(THD *thd) > +bool MYSQL_BIN_LOG::check_write_error(THD *thd) What's the difference? it's a static method anyway, FWIW it could be outside of any class. What's the point of moving it from one class to another? > { > DBUG_ENTER("MYSQL_BIN_LOG::check_write_error"); > > @@ -3806,9 +3815,9 @@ bool MYSQL_BIN_LOG::open_index_file(const char *index_file_name_arg, > } > > > -bool Event_log::open(enum cache_type io_cache_type_arg) > +bool Event_log::open(enum cache_type io_cache_type_arg, size_t buffer_size) > { > - bool error= init_io_cache(&log_file, -1, LOG_BIN_IO_SIZE, io_cache_type_arg, > + bool error= init_io_cache(&log_file, -1, buffer_size, io_cache_type_arg, why? it's only used by online alter, where would you need a different buffer size? only in your DBUG_EXECUTE_IF? > 0, 0, MYF(MY_WME | MY_NABP | MY_WAIT_IF_FULL)); > > log_state= LOG_OPENED; > @@ -6544,7 +6562,8 @@ Event_log::flush_and_set_pending_rows_event(THD *thd, Rows_log_event* event, > if (writer.write(pending)) > { > set_write_error(thd, is_transactional); > - if (check_write_error(thd) && cache_data && > + if (dynamic_cast<MYSQL_BIN_LOG*>(this) && why? > + MYSQL_BIN_LOG::check_write_error(thd) && cache_data && > stmt_has_updated_non_trans_table(thd)) > cache_data->set_incident(); > delete pending; > @@ -7721,8 +7749,11 @@ static int binlog_online_alter_end_trans(THD *thd, bool all, bool commit) > { > DBUG_ASSERT(cache.cache_log.type != READ_CACHE); > mysql_mutex_lock(binlog->get_log_lock()); > - error= binlog->write_cache(thd, &cache.cache_log); > + error= binlog->write_cache_raw(thd, &cache.cache_log); > mysql_mutex_unlock(binlog->get_log_lock()); > + > + if (unlikely(error)) > + binlog->set_write_error(my_errno); shouldn't binlog->write_cache() call set_write_error() internally? why would the caller have to do it? > } > } > else if (!commit) // rollback > @@ -7736,14 +7767,14 @@ static int binlog_online_alter_end_trans(THD *thd, bool all, bool commit) > cache.store_prev_position(); > } > > + thd->pop_internal_handler(); > > 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); > + push_warning_printf(thd, Sql_state_errno_level::WARN_LEVEL_WARN, > + ER_ERROR_ON_WRITE, ER_THD(thd, ER_ERROR_ON_WRITE), > + binlog->get_name(), errno); > + DBUG_ASSERT(binlog->get_write_error()); same comment as above about not showing ALTER problems in concurrent threads > } > } > > diff --git a/sql/log.h b/sql/log.h > index 83e323cb6fb..39512b01991 100644 > --- a/sql/log.h > +++ b/sql/log.h > @@ -444,17 +445,20 @@ class Event_log: public MYSQL_LOG > */ > class Cache_flip_event_log: public Event_log { > IO_CACHE alt_buf; > +IF_DBUG(public:,) yuck I don't see you using online_write_error or ref_count in DBUG macros > IO_CACHE *current, *alt; > std::atomic<uint> ref_count; > + std::atomic<int> online_write_error; > public: > > Cache_flip_event_log() : Event_log(), alt_buf{}, > - current(&log_file), alt(&alt_buf), ref_count(1) {} > - bool open(enum cache_type io_cache_type_arg) > + current(&log_file), alt(&alt_buf), ref_count(1), > + online_write_error(0) {} > + bool open(size_t buffer_size) looks like Cache_flip_event_log::open() doesn't need any arguments at all > { > log_file.dir= mysql_tmpdir; > alt_buf.dir= log_file.dir; > - bool res= Event_log::open(io_cache_type_arg); > + bool res= Event_log::open(WRITE_CACHE); > if (res) > return res; > > diff --git a/sql/log_event.cc b/sql/log_event.cc > index 10180a95d99..0c4c284a40f 100644 > --- a/sql/log_event.cc > +++ b/sql/log_event.cc > @@ -761,16 +761,21 @@ Log_event::Log_event(const uchar *buf, > > int Log_event::read_log_event(IO_CACHE* file, String* packet, > const Format_description_log_event *fdle, > - enum enum_binlog_checksum_alg checksum_alg_arg) > + enum enum_binlog_checksum_alg checksum_alg_arg, > + size_t max_allowed_packet) > { > ulong data_len; > char buf[LOG_EVENT_MINIMAL_HEADER_LEN]; > uchar ev_offset= packet->length(); > #if !defined(MYSQL_CLIENT) > - THD *thd=current_thd; > - ulong max_allowed_packet= thd ? thd->slave_thread ? slave_max_allowed_packet > - : thd->variables.max_allowed_packet > - : ~(uint)0; > + if (max_allowed_packet == 0) may be better pass THD as an argument? or even correct value of max_allowed_packet. As ulong max_allowed_packet(THD *thd) { return thd ? ... : ~(uint)0; } on the second thought, do you need to ignore max_allowed_packet here for online ALTER? Is there no event size limit when the event is written into binlog? > + { > + THD *thd=current_thd; > + max_allowed_packet= thd ? thd->slave_thread > + ? slave_max_allowed_packet > + : thd->variables.max_allowed_packet > + : ~(uint)0; > + } > #endif > DBUG_ENTER("Log_event::read_log_event(IO_CACHE*,String*...)"); > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 7

Re: 88a77860a8b: MDEV-31804 Assertion `thd->m_transaction_psi == __null' fails
by Sergei Golubchik 09 Aug '23

09 Aug '23

Hi, Nikita, On Aug 08, Nikita Malyavin wrote: > revision-id: 88a77860a8b (mariadb-11.0.1-194-g88a77860a8b) > parent(s): 73a677f0306 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-08-07 22:45:11 +0400 > message: > > MDEV-31804 Assertion `thd->m_transaction_psi == __null' fails > > ... upon replicating online ALTER > > When an online event is applied and slave_exec_mode is idempotent, > Write_rows_log_event::do_before_row_operations had reset > thd->lex->sql_command to SQLCOM_REPLACE. > > This led to that a statement was detected as a row-type during > binlogging, and was logged as not standalone. > > So the corresponding Gtid_log_event, when applied on replica, did not > exit early and created a new PSI transaction. Hence the difference > with non-online ALTER. Nice find. I vaguely believe I've seen in one of the commits I've reviewed a change that makes every row event set thd->lex->sql_command to the corresponding SQLCOM_INSERT/UPDATE/DELETE. But be it was in an unpushed commit. Anyway, I think it's a bit risky to assume that row events never change thd->lex->sql_command (and never will). Perhaps you should simply restore it after the applying part? Like --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -12181,6 +12181,7 @@ copy_data_between_tables(THD *thd, TABLE *from, TABLE *> if (error) from->s->tdc->flush_unused(1); // to free the binlog to->pos_in_table_list= NULL; // Safety + thd->lex->sql_command= SQLCOM_ALTER_TABLE; } else if (online) // error was on copy stage { Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 5

Re: 63c93f89af9: MDEV-31804 Assertion `thd->m_transaction_psi == __null' fails
by Sergei Golubchik 07 Aug '23

07 Aug '23

Hi, Nikita, On Aug 07, Nikita Malyavin wrote: > revision-id: 63c93f89af9 (mariadb-11.0.1-192-g63c93f89af9) > parent(s): a75ba4caa51 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-08-05 20:17:41 +0400 > message: > > MDEV-31804 Assertion `thd->m_transaction_psi == __null' fails ... > index 1c8d56cafd1..66b2b1ee4cc 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -1459,7 +1459,7 @@ void trans_register_ha(THD *thd, bool all, handlerton *ht_arg, ulonglong trxid) > Do not register transactions in which binary log is the only participating > transactional storage engine. > */ > - if (thd->m_transaction_psi == NULL && ht_arg->db_type != DB_TYPE_BINLOG) > + if (thd->m_transaction_psi == NULL && ht_arg != binlog_hton) it would've been better to fix it in 10.4, but if it doesn't show before online alter then ok > { > thd->m_transaction_psi= MYSQL_START_TRANSACTION(&thd->m_transaction_state, > thd->get_xid(), trxid, thd->tx_isolation, thd->tx_read_only, > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 88302096272..944807ea422 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -65,6 +65,7 @@ > #include "rpl_rli.h" > #include "log.h" > #include "sql_debug.h" > +#include "mysql/psi/mysql_transaction.h" > > #ifdef _WIN32 > #include <io.h> > @@ -11653,6 +11654,9 @@ bool mysql_trans_commit_alter_copy_data(THD *thd) > > DEBUG_SYNC(thd, "alter_table_copy_trans_commit"); > > + MYSQL_COMMIT_TRANSACTION(thd->m_transaction_psi); > + thd->m_transaction_psi= NULL; I don't understand it. ha_enable_transaction() commits the transaction it calls ha_commit_trans(). And ha_commit_trans() does the above psi manipulations. Why do you need to do that explicitly? Also, as a cleanup, I suspect, you can remove explicit commit that follows ha_enable_transaction() in mysql_trans_commit_alter_copy_data(). Seems to be redundant. > if (ha_enable_transaction(thd, TRUE)) > DBUG_RETURN(TRUE); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: b508107c7a3: MDEV-31631 Adding auto-increment to table with history online misbehaves
by Sergei Golubchik 04 Aug '23

04 Aug '23

Hi, Nikita, Looks good. One comment below: On Aug 04, Nikita Malyavin wrote: > revision-id: b508107c7a3 (mariadb-11.0.1-189-gb508107c7a3) > parent(s): e9a75c98781 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-07-29 00:13:48 +0400 > message: > > MDEV-31631 Adding auto-increment to table with history online misbehaves > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index eabee13e8d3..3a70ed672a6 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -9953,6 +9953,37 @@ const char *online_alter_check_supported(const THD *thd, > } > } > > + for (auto &c: alter_info->create_list) > + { > + *online= c.field || !(c.flags & AUTO_INCREMENT_FLAG); > + if (!*online) > + return "ADD COLUMN ... AUTO_INCREMENT"; > + > + auto *def= c.default_value; > + *online= !(def && def->flags & VCOL_NEXTVAL > + // either it's a new field, or a NULL -> NOT NULL change > + && (!c.field || (!(c.field->flags & NOT_NULL_FLAG) > + && (c.flags & NOT_NULL_FLAG)))); > + if (!*online) > + { > + const char *fmt= ER_THD(thd, ER_GENERATED_COLUMN_FUNCTION_IS_NOT_ALLOWED); > + > + Item::vcol_func_processor_result res; > + IF_DBUG(bool ret=,) > + def->expr->walk(&Item::check_vcol_func_processor, 0, &res); I don't think you need to walk here, res.name is always "nextval()" isn't it? > + DBUG_ASSERT(!ret); > + DBUG_ASSERT(res.errors & VCOL_NEXTVAL); > + > + LEX_CSTRING dflt{STRING_WITH_LEN("DEFAULT")}; > + size_t len= strlen(fmt) + strlen(res.name) + c.field_name.length > + + dflt.length; > + char *resp= (char*)thd->alloc(len); > + // expression %s cannot be used in the %s clause of %`s > + my_snprintf(resp, len, fmt, res.name, dflt.str, c.field_name.str); > + return resp; > + } > + } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Running msqld with thread sanitizer
by Thejaka Amila J Kanewala 04 Aug '23

04 Aug '23

Hello, I am trying to build and run a thread sanitized mysqld binary. In the CMake configuration, I see the WITH_ASAN option, however, I do not see an option to build mysqld with the thread sanitizer. After making some custom configurations, I was able to build a mysqld binary with the thread sanitizer (by linking static thread sanitizer), however, when trying to run it, I got the following error: "ThreadSanitizer: sync allocator overflow (65536*1024). Dying." This error seems to be coming from the tsan library and seems this error is caused by too many synchronizations [1]. Reducing the number of threads also didn't help me with this error. So my questions to you experts: 1. Do you run mysqld with the thread sanitizer ? 2. If yes, could you please share the details. (e.g., CMake configurations, compiler/linker flags etc.) 3. If not, any suggestions to overcome the above error ? [1] https://groups.google.com/g/thread-sanitizer/c/AX5twD_IPP4 Thank you. Best Regards, Thejaka Amila Kanewala, PhD https://github.com/thejkane/agm

2 3

Re: 44b4a3e64fa: MDEV-31777 ER_GET_ERRNO upon online alter on CONNECT table
by Sergei Golubchik 03 Aug '23

03 Aug '23

Hi, Nikita, Generally, please, prefer table flags over new virtual function. E.g. *online= new_table->file->ha_table_flags() & HA_CAN_ONLINE_ALTER; But in this particular case there's no reason for a engine to not support online alter, it's just CONNECT being very weird. Like you wrote, it doesn't support REPLACE either and we didn't introduce a flag for that. This should be solved inside CONNECT. E.g it can throw an error just like it does for REPLACE. It can upgrade the lock: --- a/storage/connect/ha_connect.cc +++ b/storage/connect/ha_connect.cc @@ -5125,10 +5125,12 @@ @see get_lock_data() in lock.cc */ -THR_LOCK_DATA **ha_connect::store_lock(THD *, +THR_LOCK_DATA **ha_connect::store_lock(THD *thd, THR_LOCK_DATA **to, enum thr_lock_type lock_type) { + if (thd_sql_command(thd) == SQLCOM_ALTER_TABLE && lock_type == TL_READ) + lock_type= TL_READ_NO_INSERT; if (lock_type != TL_IGNORE && lock.type == TL_UNLOCK) lock.type=lock_type; *to++= &lock; or it can implement reading and inserting to the same table :) Either way, I don't think we need to extend the storage engine API for this specific case. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org On Aug 03, Nikita Malyavin wrote: > revision-id: 44b4a3e64fa (mariadb-11.0.1-190-g44b4a3e64fa) > parent(s): b508107c7a3 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-07-29 00:13:48 +0400 > message: > > MDEV-31777 ER_GET_ERRNO upon online alter on CONNECT table > > Forbid Online for CONNECT. > > diff --git a/sql/handler.h b/sql/handler.h > index 480c79017ab..4a1363d2e93 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -4911,6 +4911,8 @@ class handler :public Sql_alloc > bool commit); > > > + virtual bool is_online_alter_supported() const { return true; } > + > protected:

1 0

Re: dbe5c20b755: MDEV-31482: Lock wait timeout with INSERT-SELECT, autoinc, and statement-based replication
by Kristian Nielsen 03 Aug '23

03 Aug '23

Hi Marko, I'd like to ask your opinion/review of the below patch for MDEV-31482: https://github.com/MariaDB/server/commit/dbe5c20b755b87f67d87990c3baabc8666… https://jira.mariadb.org/browse/MDEV-31482 As you know, in-order parallel replication has InnoDB report lock waits. If T1 must replicate before T2, and InnoDB reports that T1 is waiting for a lock held by T2, then T2 will be killed (and retried later) so that T1 can replicate. As an optimisation, auto-increment lock waits where _not_ reported, but this turns out to be a bug. This patch just removes the exception so that such waits are reported like other waits. My only concern is the possible performance impact of this. I'm hoping it will not be very big, because in many cases the auto-increment lock is not taken for replicated transactions? My understanding is: - It is not taken for INSERT INTO t VALUES (...) (and maybe not for multi-row "INSERT ... VALUES (...), (...), ..." ?) - SELECT ... INSERT is not safe for statement-based replication, so will be logged as row in MIXED mode and give a warning in STATEMENT. But I wanted to hear you if you know any case and/or MariaDB version where a significant amount of auto-increment lock waits could be expected in parallel replication? And if so, maybe this fix should be pushed only to later MariaDB versions? - Kristian. Kristian Nielsen <knielsen(a)knielsen-hq.org> writes: > revision-id: dbe5c20b755b87f67d87990c3baabc866667e41b (mariadb-10.4.30-2-gdbe5c20b755) > parent(s): a6114df595eeb7aeed4b050c9a3f4640c4320b5f > author: Kristian Nielsen > committer: Kristian Nielsen > timestamp: 2023-07-09 16:45:47 +0200 > message: > > MDEV-31482: Lock wait timeout with INSERT-SELECT, autoinc, and statement-based replication > > Remove the exception that InnoDB does not report auto-increment locks waits > to the parallel replication. > > There was an assumption that these waits could not cause conflicts with > in-order parallel replication and thus need not be reported. However, this > assumption is wrong and it is possible to get conflicts that lead to hangs > for the duration of --innodb-lock-wait-timeout. This can be seen with three > transactions: > > 1. T1 is waiting for T3 on an autoinc lock > 2. T2 is waiting for T1 to commit > 3. T3 is waiting on a normal row lock held by T2 > > Here, T3 needs to be deadlock killed on the wait by T1. > > Signed-off-by: Kristian Nielsen <knielsen(a)knielsen-hq.org> > > --- > mysql-test/suite/rpl/r/rpl_parallel_autoinc.result | 95 ++++++++++++++ > mysql-test/suite/rpl/t/rpl_parallel_autoinc.test | 140 +++++++++++++++++++++ > sql/sql_class.cc | 6 - > storage/innobase/lock/lock0lock.cc | 8 +- > 4 files changed, 236 insertions(+), 13 deletions(-) > > diff --git a/mysql-test/suite/rpl/r/rpl_parallel_autoinc.result b/mysql-test/suite/rpl/r/rpl_parallel_autoinc.result > new file mode 100644 > index 00000000000..c1829bafa1a > --- /dev/null > +++ b/mysql-test/suite/rpl/r/rpl_parallel_autoinc.result > @@ -0,0 +1,95 @@ > +include/master-slave.inc > +[connection master] > +MDEV-31482: Lock wait timeout with INSERT-SELECT, autoinc, and statement-based replication > +include/rpl_connect.inc [creating slave2] > +include/rpl_connect.inc [creating slave3] > +connection master; > +ALTER TABLE mysql.gtid_slave_pos ENGINE=InnoDB; > +CREATE TABLE t1 (a INT PRIMARY KEY AUTO_INCREMENT, b INT, c INT, INDEX (c)) ENGINE=InnoDB; > +INSERT INTO t1 (b,c) VALUES (0, 1), (0, 1), (0, 2), (0,3), (0, 5), (0, 7), (0, 8); > +CREATE TABLE t2 (a INT PRIMARY KEY, b INT) ENGINE=InnoDB; > +INSERT INTO t2 VALUES (10,1), (20,2), (30,3), (40,4), (50,5); > +CREATE TABLE t3 (a VARCHAR(20) PRIMARY KEY, b INT) ENGINE=InnoDB; > +INSERT INTO t3 VALUES ('row for T1', 0), ('row for T2', 0), ('row for T3', 0); > +include/save_master_gtid.inc > +connection slave; > +include/sync_with_master_gtid.inc > +include/stop_slave.inc > +set @@global.slave_parallel_threads= 3; > +set @@global.slave_parallel_mode= OPTIMISTIC; > +set @@global.innodb_lock_wait_timeout= 20; > +connection master; > +BEGIN; > +UPDATE t3 SET b=b+1 where a="row for T1"; > +INSERT INTO t1(b, c) SELECT 1, t2.b FROM t2 WHERE a=10; > +Warnings: > +Note 1592 Unsafe statement written to the binary log using statement format since BINLOG_FORMAT = STATEMENT. Statements writing to a table with an auto-increment column after selecting from another table are unsafe because the order in which rows are retrieved determines what (if any) rows will be written. This order cannot be predicted and may differ on master and the slave > +COMMIT; > +DELETE FROM t1 WHERE c >= 4 and c < 6; > +BEGIN; > +UPDATE t3 SET b=b+1 where a="row for T3"; > +INSERT INTO t1(b, c) SELECT 3, t2.b FROM t2 WHERE a >= 20 AND a <= 40; > +Warnings: > +Note 1592 Unsafe statement written to the binary log using statement format since BINLOG_FORMAT = STATEMENT. Statements writing to a table with an auto-increment column after selecting from another table are unsafe because the order in which rows are retrieved determines what (if any) rows will be written. This order cannot be predicted and may differ on master and the slave > +COMMIT; > +include/save_master_gtid.inc > +connection slave1; > +BEGIN; > +SELECT * FROM t3 WHERE a="row for T1" FOR UPDATE; > +a b > +row for T1 0 > +connection slave2; > +BEGIN; > +SELECT * FROM t3 WHERE a="row for T3" FOR UPDATE; > +a b > +row for T3 0 > +connection slave3; > +BEGIN; > +DELETE FROM t2 WHERE a=30; > +connection slave; > +include/start_slave.inc > +connection slave2; > +ROLLBACK; > +connection slave1; > +ROLLBACK; > +connection slave3; > +ROLLBACK; > +connection slave; > +include/sync_with_master_gtid.inc > +SELECT * FROM t1 ORDER BY a; > +a b c > +1 0 1 > +2 0 1 > +3 0 2 > +4 0 3 > +6 0 7 > +7 0 8 > +8 1 1 > +9 3 2 > +10 3 3 > +11 3 4 > +SELECT * FROM t2 ORDER BY a; > +a b > +10 1 > +20 2 > +30 3 > +40 4 > +50 5 > +SELECT * FROM t3 ORDER BY a; > +a b > +row for T1 1 > +row for T2 0 > +row for T3 1 > +connection master; > +CALL mtr.add_suppression("Unsafe statement written to the binary log using statement format"); > +DROP TABLE t1, t2, t3; > +connection slave; > +include/stop_slave.inc > +SET @@global.slave_parallel_threads= 0; > +SET @@global.slave_parallel_mode= conservative; > +SET @@global.innodb_lock_wait_timeout= 50; > +include/start_slave.inc > +SELECT @@GLOBAL.innodb_autoinc_lock_mode; > +@@GLOBAL.innodb_autoinc_lock_mode > +1 > +include/rpl_end.inc > diff --git a/mysql-test/suite/rpl/t/rpl_parallel_autoinc.test b/mysql-test/suite/rpl/t/rpl_parallel_autoinc.test > new file mode 100644 > index 00000000000..0e96b4dfb80 > --- /dev/null > +++ b/mysql-test/suite/rpl/t/rpl_parallel_autoinc.test > @@ -0,0 +1,140 @@ > +--source include/have_binlog_format_statement.inc > +--source include/have_innodb.inc > +--source include/master-slave.inc > + > +--echo MDEV-31482: Lock wait timeout with INSERT-SELECT, autoinc, and statement-based replication > + > +# The scenario is transactions T1, T2, T3: > +# > +# T1 is waiting for T3 on an autoinc lock > +# T2 is waiting for T1 to commit > +# T3 is waiting on a normal row lock held by T2 > +# > +# This caused a hang until innodb_lock_wait_timeout, because autoinc > +# locks were not reported to the in-order parallel replication, so T3 > +# was not deadlock killed. > + > +--let $lock_wait_timeout=20 > + > +--let $rpl_connection_name= slave2 > +--let $rpl_server_number= 2 > +--source include/rpl_connect.inc > + > +--let $rpl_connection_name= slave3 > +--let $rpl_server_number= 2 > +--source include/rpl_connect.inc > + > +--connection master > +ALTER TABLE mysql.gtid_slave_pos ENGINE=InnoDB; > + > +# A table as destination for INSERT-SELECT > +CREATE TABLE t1 (a INT PRIMARY KEY AUTO_INCREMENT, b INT, c INT, INDEX (c)) ENGINE=InnoDB; > +INSERT INTO t1 (b,c) VALUES (0, 1), (0, 1), (0, 2), (0,3), (0, 5), (0, 7), (0, 8); > + > +# A table as source for INSERT-SELECT. > +CREATE TABLE t2 (a INT PRIMARY KEY, b INT) ENGINE=InnoDB; > +INSERT INTO t2 VALUES (10,1), (20,2), (30,3), (40,4), (50,5); > + > +# A table to help order slave worker threads to setup the desired scenario. > +CREATE TABLE t3 (a VARCHAR(20) PRIMARY KEY, b INT) ENGINE=InnoDB; > +INSERT INTO t3 VALUES ('row for T1', 0), ('row for T2', 0), ('row for T3', 0); > +--source include/save_master_gtid.inc > + > +--connection slave > +--source include/sync_with_master_gtid.inc > +--source include/stop_slave.inc > +--let $save_innodb_lock_wait_timeout= `SELECT @@global.innodb_lock_wait_timeout` > +--let $save_slave_parallel_threads= `SELECT @@global.slave_parallel_threads` > +--let $save_slave_parallel_mode= `SELECT @@global.slave_parallel_mode` > +set @@global.slave_parallel_threads= 3; > +set @@global.slave_parallel_mode= OPTIMISTIC; > +eval set @@global.innodb_lock_wait_timeout= $lock_wait_timeout; > + > +--connection master > +# Transaction T1. > +BEGIN; > +UPDATE t3 SET b=b+1 where a="row for T1"; > +INSERT INTO t1(b, c) SELECT 1, t2.b FROM t2 WHERE a=10; > +COMMIT; > + > +# Transaction T2. > +DELETE FROM t1 WHERE c >= 4 and c < 6; > + > +# Transaction T3. > +BEGIN; > +UPDATE t3 SET b=b+1 where a="row for T3"; > +INSERT INTO t1(b, c) SELECT 3, t2.b FROM t2 WHERE a >= 20 AND a <= 40; > +COMMIT; > + > +--source include/save_master_gtid.inc > + > +--connection slave1 > +# Temporarily block T1 to create the scheduling that triggers the bug. > +BEGIN; > +SELECT * FROM t3 WHERE a="row for T1" FOR UPDATE; > + > +--connection slave2 > +# Temporarily block T3 from starting (so T2 can reach commit). > +BEGIN; > +SELECT * FROM t3 WHERE a="row for T3" FOR UPDATE; > + > +--connection slave3 > +# This critical step blocks T3 after it has inserted its first row, > +# and thus taken the auto-increment lock, but before it has reached > +# the point where it gets a row lock wait on T2. Even though > +# auto-increment lock waits were not reported due to the bug, > +# transitive lock waits (T1 waits on autoinc of T3 which waits on row > +# on T2) _were_ reported as T1 waiting on T2, and thus a deadlock kill > +# happened and the bug was not triggered. > +BEGIN; > +DELETE FROM t2 WHERE a=30; > + > +--connection slave > +--source include/start_slave.inc > + > +# First let T2 complete until it is waiting for T1 to commit. > +--let $wait_condition= SELECT count(*)=1 FROM information_schema.processlist WHERE state='Waiting for prior transaction to commit' and command LIKE 'Slave_worker'; > +--source include/wait_condition.inc > + > +# Then let T3 reach the point where it has obtained the autoinc lock, > +# but it is not yet waiting for a row lock held by T2. > +--connection slave2 > +ROLLBACK; > +--let $wait_condition= SELECT count(*)=1 FROM information_schema.processlist WHERE state='Sending data' and info LIKE 'INSERT INTO t1(b, c) SELECT 3, t2.b%' and time_ms > 500 and command LIKE 'Slave_worker'; > +--source include/wait_condition.inc > + > +# Now let T1 continue, while T3 is holding the autoinc lock but before > +# it is waiting for T2. Wait a short while to give the hang a chance to > +# happen; T1 needs to get to request the autoinc lock before we let T3 > +# continue. (There's a small chance the sleep will be too small, which will > +# let the test occasionally pass on non-fixed server). > +--connection slave1 > +ROLLBACK; > +--sleep 0.5 > + > +# Now let T3 continue; the bug was that this lead to an undetected > +# deadlock that remained until innodb lock wait timeout. > +--connection slave3 > +ROLLBACK; > + > +--connection slave > +--let $slave_timeout= `SELECT $lock_wait_timeout/2` > +--source include/sync_with_master_gtid.inc > +--let $slave_timeout= > +SELECT * FROM t1 ORDER BY a; > +SELECT * FROM t2 ORDER BY a; > +SELECT * FROM t3 ORDER BY a; > + > +# Cleanup. > +--connection master > +CALL mtr.add_suppression("Unsafe statement written to the binary log using statement format"); > +DROP TABLE t1, t2, t3; > + > +--connection slave > +--source include/stop_slave.inc > +eval SET @@global.slave_parallel_threads= $save_slave_parallel_threads; > +eval SET @@global.slave_parallel_mode= $save_slave_parallel_mode; > +eval SET @@global.innodb_lock_wait_timeout= $save_innodb_lock_wait_timeout; > +--source include/start_slave.inc > +SELECT @@GLOBAL.innodb_autoinc_lock_mode; > +--source include/rpl_end.inc > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 73bb654080a..8ed3f8a9c5e 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -5119,12 +5119,6 @@ thd_need_wait_reports(const MYSQL_THD thd) > deadlock with the pre-determined commit order, we kill the later > transaction, and later re-try it, to resolve the deadlock. > > - This call need only receive reports about waits for locks that will remain > - until the holding transaction commits. InnoDB auto-increment locks, > - for example, are released earlier, and so need not be reported. (Such false > - positives are not harmful, but could lead to unnecessary kill and retry, so > - best avoided). > - > Returns 1 if the OTHER_THD will be killed to resolve deadlock, 0 if not. The > actual kill will happen later, asynchronously from another thread. The > caller does not need to take any actions on the return value if the > diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc > index 26388ad95e2..b1cf2152cd6 100644 > --- a/storage/innobase/lock/lock0lock.cc > +++ b/storage/innobase/lock/lock0lock.cc > @@ -6872,13 +6872,7 @@ DeadlockChecker::search() > return m_start; > } > > - /* We do not need to report autoinc locks to the upper > - layer. These locks are released before commit, so they > - can not cause deadlocks with binlog-fixed commit > - order. */ > - if (m_report_waiters > - && (lock_get_type_low(lock) != LOCK_TABLE > - || lock_get_mode(lock) != LOCK_AUTO_INC)) { > + if (m_report_waiters) { > thd_rpl_deadlock_check(m_start->mysql_thd, > lock->trx->mysql_thd); > }

2 1

Proposal: mariadbd should automatically upgrade system tables and data directory on startup
by Otto Kekäläinen 03 Aug '23

03 Aug '23

Hi! I frequently see users running into various problems after upgrading MariaDB and starting the upgraded mariadbd server: - Server might refuse to start, asking users to manually run mariadb-upgrade - Server might start, but misbehave as on-disk data format for regular tables or the system tables has not updated - On systems where mariadb-upgrade runs automatically on MariaDB service startup there might be unnecessary delay if mariadb-upgrade is run multiple times - If running mariadb-upgrade fails or is slow, users might end up in a situation where they re-restart the server and have two mariadb-upgrade scripts running and blocking each other - In some cases user might attempt to run mariadb-upgrade but it fails it the MariaDB server was not running, forcing user to figure out how to manually start the server for upgrades - Users might end up starting server in a mode that does not accept connections with the intent to only run mariadb-upgrade, but then don't understand they need to restart the server to have it serve connections again - Users planning upgrades get confused because because reading about Fedora based distros the DBA is requires to manually run mariadb-upgrade themselves, and reading about Debian based distros DBAs are not required to do this as it is automatic, but they might end up trying to run it manually anyway because of prior experience with Fedora Some of the above issues can be remediated by running 'mariadb_upgrade --check-if-upgrade-is-needed' before actually running mariadb_upgrade in service scripts, but it does not work if server is offline (https://jira.mariadb.org/browse/MDEV-27636) Making users aware that their database is running but mariadb-upgrade has not run can be done via https://github.com/MariaDB/server/pull/2464. However this are just duct tape to the original problem. All of the above could be permanently solved if mariadbd simply did the upgrade by itself automatically. Can we please have this? Can we agree that this would be a desired feature in next MariaDB 11.x version, and if somebody implements this, it would be timely reviewed and merged? There is already design outlined at https://jira.mariadb.org/browse/MDEV-30499

8 19