July 2023 - developers - lists.mariadb.org

Client-server protocol improvement proposal: Connection redirection
by Lenski, Daniel 18 Aug '23

18 Aug '23

Hi! I would like to propose a new feature in the MariaDB client-server protocol<https://mariadb.com/kb/en/clientserver-protocol>: application-layer redirection of client connections. We want the MariaDB server to be able to tell clients connecting to it, “Sorry, this server is unavailable. Connect to an alternate server instead.” This mechanism is inspired by HTTP 302 (“temporary redirect”) mechanism familiar to all developers of web applications, and is intended to have similar semantics and security properties, since these have now been widely deployed and tested for decades. I have submitted a minimal but viable implementation of this at: * https://github.com/MariaDB/server/pull/2681, server-side implementation * https://github.com/mariadb-corporation/mariadb-connector-c/pull/226, MariaDB Connector/C implementation I am seeking advice and opinions on these, and will proceed to finalize this in the following 1-2 months. In its current form, this implementation allows the MariaDB server administrator to set two variables: * SET GLOBAL SERVER_REDIRECT_MODE={ON,OFF} (the default is OFF) * SET GLOBAL SERVER_REDIRECT_TARGET='my-new-server.example.com' (or 192.168.0.123:3307, or new-server.com:3308, etc) When SERVER_REDIRECT_MODE is set to on, the server will stop accepting new TCP-based connections (unless they are to the extra port<https://mariadb.com/kb/en/thread-pool-in-mariadb/#configuring-the-extra-port>, which is intended for emergency administrative use), and it will instead respond to all clients with an error packet<https://mariadb.com/kb/en/err_packet/>: $ mariadb --host my-server.example.com ERROR 4189 (HY000): |Server is redirecting clients to 'my-new-server.example.com:3307'|my-new-server.example.com:3307 When an appropriately-updated client receives this error packet (error code 4196, message formatted as |Human readable message|<value of SERVER_REDIRECT_TARGET>), it will parse the redirection target out from the error message and attempt a new connection: $ updated-mariadb --host my-server.example.com -e 'select @@hostname' Got server redirect to 'my-new-server.example.com' (port 3307) +---------------------------+ | @@hostname | +---------------------------+ | my-new-server.example.com | +---------------------------+ The feature is gracefully backwards-compatible. Old clients that do not support redirection will fail and show the message in a form from which humans can deduce what happened. New clients that do support it will follow it to the new server. The feature is also designed to be extensible in a forwards-compatible way. The SERVER_REDIRECT_MODE system variable could be expanded to support additional redirection modes. For example, it could be augmented to redirect new connections only from specific IP subnets, or to direct the client to interpret the SERVER_REDIRECT_TARGET variable in a new form (such as a<https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-reference-jdbc-url…> JDBC-compatible mysql:// URL<https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-reference-jdbc-url…> as proposed in MDEV-15935<https://jira.mariadb.org/browse/MDEV-15935>). While researching and designing this connection redirection mechanism, I discovered or rediscovered a couple of major TLS-related security vulnerabilities in the MariaDB Connector/C client [1, 2]. These vulnerabilities affect all MariaDB users who care about the confidentiality and authenticity of their client/server connections, and they become even more severe if a connection redirection mechanism exists — because this entails the client trusting the server to send it instructions for creating a new network connection. Unless these are resolved, any connection redirection mechanism will be susceptible to hijacking and redirection to an attacker-controlled server even if the client thinks it is using TLS/SSL. What are the use cases for this connection redirection feature? Even in this simple, initial form, the feature is already useful for database administrators who want to gracefully shut down a MariaDB server (for example, to upgrade it), and to redirect new client connections to a replica or alternate server that can accept the traffic. Server-side connection redirection can be combined with existing MariaDB server features, such as SET GLOBAL READ_ONLY=ON and KILLing of idle or long-running connections, in order to build a full switchover solution for shunting traffic from one server to another as gracefully as possible. Currently, MariaDB database administrators tend to use proxies and wrappers (such as HikariCP) to redirect client connections from one MariaDB server to another. Supporting this redirection mechanism directly in the server, and in the client-server protocol, will allow many simple use cases of redirection without extra software, making MariaDB easier and simpler to manage for minimal downtime situations. Please post on this mailing list what you think about this suggestion. I am looking forward to polishing this feature and submitting it in a form that meets the quality requirements for new features from MariaDB core developers. Thank you! Daniel Lenski Amazon RDS MySQL/MariaDB engine team --- Notes on TLS vulnerabilities in MariaDB Connector/C: [1] CONC-648<https://jira.mariadb.org/browse/CONC-648>, reported by me on 5 June: when a Connector/C-based client is using connections secured by TLS/SSL, Connector/C will nevertheless receive plain-text, unencrypted error packets prior to the TLS handshake, and present them to the client in a way that’s indistinguishable from TLS-protected error packets sent by the server after the TLS handshake. This longstanding vulnerability is a consequence of the tangled and unsystematic way that the Connector/C codebase interleaves the MariaDB application-layer authentication protocol with the transport-layer security protocol. My standalone mariadb-connector-c PR#223<https://github.com/mariadb-corporation/mariadb-connector-c/pull/223> fixes the most straightforward manifestation of this vulnerability. Until this issue is resolved in Connector/C, it will be trivial for MITM attackers to inject plaintext connection redirection packets into MariaDB client/server connections, and thereby redirect clients to attacker-controlled servers despite those clients’ intention to use only TLS-secured connections. [2] MDEV-28634<https://jira.mariadb.org/browse/MDEV-28634>, reported almost 3 years ago by Geoff Montee: Connector/C clients which specify --ssl without --ssl-verify-server-cert will allow their connections to be silently downgraded to unencrypted, plaintext connections. This vulnerability can be used by on-path attackers to trivially downgrade most MariaDB client-server connections from TLS-encrypted to plaintext. My standalone mariadb-connector-c PR#224<https://github.com/mariadb-corporation/mariadb-connector-c/pull/224> fixes this. It is a backwards-incompatible change in that clients which specify --ssl (and thus clearly expect TLS-secured connections) will no longer succeed in connecting to servers which don’t support TLS. This should be considered a good thing: MariaDB Connector/C-based clients have been misleading their users about whether their connections are guaranteed to be encrypted, and will no longer do so. Essentially all software that purports to secure network connections with TLS have implemented similar changes, most of it about a decade ago when the pervasiveness of such attacks<https://en.wikipedia.org/wiki/Global_surveillance_disclosures_(2013%E2%80%9…> (by ISPs, censors, intelligenceagencies, etc) became very widely-known.

8 28

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: 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: 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

Problem Upgrading Database
by jashley＠deadacated.com 25 Jul '23

25 Jul '23

Greetings, MariaDB 10.11.4 on Server 2022 Upgrading from 10.11.4 to 11.0.2 or 11.1.1 fails on Phase 5/10: Waiting for startup to complete. Is this a known problem? Thank you, mux

3 2

Re: 64ac6ed3e35: MDEV-31742 incorrect examined rows in case of stored function usage
by Sergei Golubchik 21 Jul '23

21 Jul '23

Hi, Oleksandr, On Jul 21, Oleksandr Byelkin wrote: > revision-id: 64ac6ed3e35 (mariadb-10.4.30-31-g64ac6ed3e35) > parent(s): 55a27bac80c > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2023-07-19 16:14:39 +0200 > message: > > MDEV-31742 incorrect examined rows in case of stored function usage > > The counter is global so we do not need add backup to it > if we do not zero it after taking the backup. > > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index c9e86b6142c..7911c78b6e0 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -5568,7 +5568,6 @@ void THD::restore_sub_statement_state(Sub_statement_state *backup) > The following is added to the old values as we are interested in the > total complexity of the query > */ > - inc_examined_row_count(backup->examined_row_count); may be backup->examined_row_count should be simply removed? Sub_statement_state::examined_row_count, I mean. Not saved, not restored? There's also void THD::add_slow_query_state(Sub_statement_state *backup) { affected_rows+= backup->affected_rows; bytes_sent_old= backup->bytes_sent_old; m_examined_row_count+= backup->examined_row_count; it's not even using inc_examined_row_count(). Also examined_row_count is mentioned in comments in sql_class.cc, if you'll be removing it from Sub_statement_state, don't forget to fix comments to match. > cuted_fields+= backup->cuted_fields; > DBUG_VOID_RETURN; > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: 08403d85b00: MDEV-31677 Assertion failed upon online ALTER with binlog_row_image=NOBLOB
by Sergei Golubchik 21 Jul '23

21 Jul '23

Hi, Nikita, ok to push after making few small changes, see inline: On Jul 20, Nikita Malyavin wrote: > revision-id: 08403d85b00 (mariadb-11.0.1-147-g08403d85b00) > parent(s): a31b2a52119 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-07-19 19:08:51 +0400 > message: > > MDEV-31677 Assertion failed upon online ALTER with binlog_row_image=NOBLOB > > Make binlog_prepare_row_images accept image type as an argument. > > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index d4ad0efbf3a..bb2453af6a0 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -7070,7 +7071,7 @@ int THD::binlog_update_row(TABLE* table, Event_log *bin_log, > not needed for binlogging. This is done according to the: > binlog-row-image option. the comment is outdated, should say This is done according to the row_image argument > */ > - binlog_prepare_row_images(table); > + binlog_prepare_row_images(table, row_image); > > size_t const before_maxlen= max_row_length(table, table->read_set, > before_record); > @@ -7194,7 +7195,7 @@ void THD::binlog_prepare_row_images(TABLE *table) > and the handler involved supports this. > */ > if (table->s->primary_key < MAX_KEY && > - (thd->variables.binlog_row_image < BINLOG_ROW_IMAGE_FULL) && > + row_image < BINLOG_ROW_IMAGE_FULL && > !ha_check_storage_engine_flag(table->s->db_type(), > HTON_NO_BINLOG_ROW_OPT)) > { please also repace thd->variables.binlog_row_image in the switch() below. doesn't make any practical difference, just for clarity Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Debugging MDEV-28776, the rare rpl.rpl_mark_optimize_tbl_ddl test failure
by Kristian Nielsen 21 Jul '23

21 Jul '23

Hi Brandon, The test failure in https://jira.mariadb.org/browse/MDEV-28776 (the one with 10 failed retries after deadlock, not the freebsd failures) seems like it could be something serious. But it has been very difficult to track down. I believe you have tried different things, and I also tried hard to reproduce it, so far without success. But it's still failing occasionally in actual buildbot runs. What do you think about pushing the below to the different branches? It makes the test case log InnoDB information about deadlocks encountered. This way, when it fails the next time in buildbot we can see 1) what kind of InnoDB deadlock is triggering the problem, if any; and 2) if it actually gets a deadlock inside InnoDB 10 times in a row, or if it is something else. - Kristian. ----------------------------------------------------------------------- Author: Kristian Nielsen <knielsen(a)knielsen-hq.org> Date: Sun Jul 9 15:18:03 2023 +0200 MDEV-28776: rpl.rpl_mark_optimize_tbl_ddl fails with timeout on sync_with_master This commit just extends the testcase to include some more information when the test failure occurs. The failure has so far only been reproducible in buildbot runs. Signed-off-by: Kristian Nielsen <knielsen(a)knielsen-hq.org> ----------------------------------------------------------------------- diff --git a/mysql-test/suite/rpl/t/rpl_mark_optimize_tbl_ddl.opt b/mysql-test/suite/rpl/t/rpl_mark_optimize_tbl_ddl.opt new file mode 100644 index 00000000000..3c53d5257b4 --- /dev/null +++ b/mysql-test/suite/rpl/t/rpl_mark_optimize_tbl_ddl.opt @@ -0,0 +1 @@ +--innodb-print-all-deadlocks=1

3 6

Re: 534031b77e0: MDEV-31400 Simple plugin dependency resolution
by Sergei Golubchik 20 Jul '23

20 Jul '23

Hi, Yuchen, Thanks. Conceptually all good, the approach is correct. See comments below - about the state and about user visible warnings, nothing major. On Jul 18, Yuchen Pei wrote: > revision-id: 534031b77e0 (mariadb-10.9.5-17-g534031b77e0) > parent(s): d8997f875e2 > author: Yuchen Pei > committer: Yuchen Pei > timestamp: 2023-07-03 17:54:56 +1000 > message: > > MDEV-31400 Simple plugin dependency resolution > > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index 5a077a934ac..15717352040 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -1435,6 +1435,47 @@ void plugin_unlock_list(THD *thd, plugin_ref *list, size_t count) > DBUG_VOID_RETURN; > } > > +static int plugin_do_initialize(struct st_plugin_int *plugin, uint &state) > +{ > + DBUG_ENTER("plugin_do_initialize"); > + mysql_mutex_assert_not_owner(&LOCK_plugin); > + plugin_type_init init= plugin_type_initialize[plugin->plugin->type]; > + if (!init) > + init= (plugin_type_init) plugin->plugin->init; > + if (init) > + if (int ret= init(plugin)) > + { > + /* Plugin init failed but requested a retry if possible */ > + if (unlikely(ret== HA_ERR_RETRY_INIT)) > + sql_print_warning("Plugin '%s' registration as a %s failed.", > + plugin->name.str, plugin_type_names[plugin->plugin->type].str); > + else > + sql_print_error("Plugin '%s' registration as a %s failed.", > + plugin->name.str, plugin_type_names[plugin->plugin->type].str); > + DBUG_RETURN(ret); > + } > + state= PLUGIN_IS_READY; // plugin->init() succeeded > + > + if (plugin->plugin->status_vars) > + { > + /* > + historical ndb behavior caused MySQL plugins to specify > + status var names in full, with the plugin name prefix. > + this was never fixed in MySQL. > + MariaDB fixes that but supports MySQL style too. > + */ > + SHOW_VAR *show_vars= plugin->plugin->status_vars; > + SHOW_VAR tmp_array[2]= {{plugin->plugin->name, > + (char *) plugin->plugin->status_vars, SHOW_ARRAY}, > + {0, 0, SHOW_UNDEF}}; > + if (strncasecmp(show_vars->name, plugin->name.str, plugin->name.length)) > + show_vars= tmp_array; > + > + if (add_status_vars(show_vars)) > + DBUG_RETURN(1); > + } > + DBUG_RETURN(0); > +} > > static int plugin_initialize(MEM_ROOT *tmp_root, struct st_plugin_int *plugin, > int *argc, char **argv, bool options_only) > @@ -1595,7 +1594,7 @@ int plugin_init(int *argc, char **argv, int flags) > size_t i; > struct st_maria_plugin **builtins; > struct st_maria_plugin *plugin; > - struct st_plugin_int tmp, *plugin_ptr, **reap; > + struct st_plugin_int tmp, *plugin_ptr, **reap, **retry_end, **retry_start; > MEM_ROOT tmp_root; > bool reaped_mandatory_plugin= false; > bool mandatory= true; > @@ -1737,11 +1736,16 @@ int plugin_init(int *argc, char **argv, int flags) > */ > > mysql_mutex_lock(&LOCK_plugin); > + /* List of plugins to reap */ > reap= (st_plugin_int **) my_alloca((plugin_array.elements+1) * sizeof(void*)); > *(reap++)= NULL; > + /* List of plugins to retry */ > + retry_start= retry_end= > + (st_plugin_int **) my_alloca((plugin_array.elements+1) * sizeof(void*)); > > for(;;) > { > + int error; > for (i=0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) > { > HASH *hash= plugin_hash + plugin_type_initialization_order[i]; > @@ -1753,16 +1757,46 @@ int plugin_init(int *argc, char **argv, int flags) > bool plugin_table_engine= lex_string_eq(&plugin_table_engine_name, > &plugin_ptr->name); > bool opts_only= flags & PLUGIN_INIT_SKIP_INITIALIZATION && > - (flags & PLUGIN_INIT_SKIP_PLUGIN_TABLE || > - !plugin_table_engine); > - if (plugin_initialize(&tmp_root, plugin_ptr, argc, argv, opts_only)) > + (flags & PLUGIN_INIT_SKIP_PLUGIN_TABLE || !plugin_table_engine); > + error= plugin_initialize(&tmp_root, plugin_ptr, argc, argv, > + opts_only); > + if (error) > { > plugin_ptr->state= PLUGIN_IS_DYING; > - *(reap++)= plugin_ptr; > + /* The plugin wants a retry of the initialisation, > + possibly due to dependency on other plugins */ > + if (unlikely(error == HA_ERR_RETRY_INIT)) > + *(retry_end++)= plugin_ptr; > + else > + *(reap++)= plugin_ptr; > } > } > } > } > + /* Retry plugins that asked for it */ > + while (retry_start < retry_end) > + { > + st_plugin_int **to_re_retry, **retrying; > + for (to_re_retry= retrying= retry_start; retrying < retry_end; retrying++) > + { > + plugin_ptr= *retrying; > + uint state= plugin_ptr->state; > + mysql_mutex_unlock(&LOCK_plugin); you might want to add here sql_print_information("plugin XXX registration is retried") otherwise it would be confusing to see "plugin XXX registration failed" and then see the plugin perferctly initialized and usable. Alternatively (preferable, even) don't print "plugin registration failed" above, print it when adding a plugin to the reap list. So that the user won't see fail/retry loop at all, this is the inner working of the plugin subsystem, no user serviceable parts inside. > + error= plugin_do_initialize(plugin_ptr, state); > + mysql_mutex_lock(&LOCK_plugin); > + plugin_ptr->state= state; why do you need to manipulate (save/restore) the state here? Is that even correct? You set the state to PLUGIN_IS_DYING earlier and plugin_do_initialize() an change it to PLUGIN_IS_READY on success. You wouldn't want to change PLUGIN_IS_READY to PLUGIN_IS_DYING, would you? If this is the case, then you don't need to save the state at all, instead you need something like if (error) plugin_ptr->state= PLUGIN_IS_DYING; > + if (error == HA_ERR_RETRY_INIT) > + *(to_re_retry++)= plugin_ptr; > + else if (error) > + *(reap++)= plugin_ptr; > + } > + /* If the retry list has not changed, i.e. if all retry attempts > + result in another retry request, empty the retry list */ > + if (to_re_retry == retry_end) > + while (to_re_retry > retry_start) > + *(reap++)= *(--to_re_retry); > + retry_end= to_re_retry; > + } > > /* load and init plugins from the plugin table (unless done already) */ > if (flags & PLUGIN_INIT_SKIP_PLUGIN_TABLE) > @@ -1775,7 +1809,8 @@ int plugin_init(int *argc, char **argv, int flags) > } > > /* > - Check if any plugins have to be reaped > + Merge the retry list to the reap list, then reap the failed > + plugins. Note that during the merge we reverse the order in retry seems like this comment is obsolete > */ > while ((plugin_ptr= *(--reap))) > { > @@ -1788,6 +1823,7 @@ int plugin_init(int *argc, char **argv, int flags) > } > > mysql_mutex_unlock(&LOCK_plugin); > + my_afree(retry_start); > my_afree(reap); > if (reaped_mandatory_plugin && !opt_help) > goto err; > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1