August 2023 - developers - lists.mariadb.org

Re: 82a49812507: MDEV-22597 Add views for periods in information_schema
by Sergei Golubchik 05 Sep '23

05 Sep '23

Hi, Nikita, On Aug 31, Nikita Malyavin wrote: > revision-id: 82a49812507 (mariadb-11.0.1-228-g82a49812507) > parent(s): 23b15d24eb3 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-08-30 22:49:36 +0400 > message: > > MDEV-22597 Add views for periods in information_schema > > Two new information_schema views are added: > * PERIOD table -- columns TABLE_CATALOG, TABLE_SCHEMA, TABLE_NAME, > PERIOD_NAME, START_COLUMN_NAME, END_COLUMN_NAME. > * KEY_PERIOD_USAGE -- works similar to KEY_COLUMN_USAGE, but for periods. > Columns CONSTRAINT_CATALOG, CONSTRAINT_SCHEMA, CONSTRAINT_NAME, > TABLE_CATALOG, TABLE_SCHEMA, TABLE_NAME, PERIOD_NAME > > Two new columns are added to the COLUMNS view: > IS_SYSTEM_TIME_PERIOD_START, IS_SYSTEM_TIME_PERIOD_END - contain YES/NO. > > diff --git a/mysql-test/main/information_schema-big.result b/mysql-test/main/information_schema-big.result > index 94c8d274fad..ad0451293c9 100644 > --- a/mysql-test/main/information_schema-big.result > +++ b/mysql-test/main/information_schema-big.result good, you didn't forget to update big and embedded tests > diff --git a/mysql-test/suite/period/r/i_s_notembedded.result b/mysql-test/suite/period/r/i_s_notembedded.result > new file mode 100644 > index 00000000000..879376cc8b4 > --- /dev/null > +++ b/mysql-test/suite/period/r/i_s_notembedded.result > @@ -0,0 +1,52 @@ > +select * from information_schema.periods; > +TABLE_CATALOG TABLE_SCHEMA TABLE_NAME PERIOD START_COLUMN_NAME END_COLUMN_NAME > +create or replace table t1 (id int primary key, s timestamp(6), e timestamp(6), > +period for mytime(s,e)); > +create or replace table t2 (id int primary key, s timestamp(6), e timestamp(6), > +period for mytime(s,e)) with system versioning; > +show columns from t1; > +Field Type Null Key Default Extra > +id int(11) NO PRI NULL > +s timestamp(6) NO NULL > +e timestamp(6) NO NULL > +select * from information_schema.periods where table_schema = 'test'; > +TABLE_CATALOG TABLE_SCHEMA TABLE_NAME PERIOD START_COLUMN_NAME END_COLUMN_NAME > +def test t2 SYSTEM_TIME row_start row_end I don't think so, they're INVISIBLE_SYSTEM. for all practical purposes they don't exist as columns in the table. Shouldn't be shown either. But don't remove this test, let's keep it to show that row_start/row_end are *not* shown here. And add table t3 with full standard definition of period for system_time. it should be shown all right. > +def test t2 mytime s e > +def test t1 mytime s e > +create user periods_hidden@localhost; ... > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 1dc60a01f38..876d6c6f244 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -6165,11 +6166,83 @@ static void store_variable_type(THD *thd, const sp_variable *spvar, > } > } > > +static int store_schema_period_record(THD *thd, TABLE_LIST *tl, > + TABLE *schema_table, > + const LEX_CSTRING *db_name, > + const LEX_CSTRING *table_name, > + const TABLE_SHARE::period_info_t &period) > +{ > + TABLE_SHARE *s= tl->table->s; > + const CHARSET_INFO *cs= system_charset_info; > + > + int err= schema_table->field[0]->store(STRING_WITH_LEN("def"), cs); > + if(!err) err= schema_table->field[1]->store(db_name, cs); > + if(!err) err= schema_table->field[2]->store(table_name, cs); > + if(!err) err= schema_table->field[3]->store(period.name, cs); well, ok. but as you've seen elsewhere these go without checks, as these calls generally cannot fail, they don't even allocate any memory. > + > + if (err) return err; > + > + int period_field= 4; > + for (auto *field: {period.start_field(s), period.end_field(s)}) > + { > +#ifndef NO_EMBEDDED_ACCESS_CHECKS > + bool col_granted= get_column_grant(thd, &tl->grant, > + db_name->str, table_name->str, > + field->field_name.str) & COL_ACLS; > + if (col_granted) > +#endif > + { > + schema_table->field[period_field]->set_notnull(); > + err= schema_table->field[period_field]->store(field->field_name, cs); > + } > + > + if (err) > + return err; > + period_field++; > + } > + > + return schema_table_store_record(thd, schema_table); > +} > + > + > +static int > +get_schema_period_records(THD *thd, TABLE_LIST *tl, > + TABLE *schema_table, //!< @ref > + //!< Show::periods_fields_info > + bool res, > + const LEX_CSTRING *db_name, > + const LEX_CSTRING *table_name) > +{ > + TABLE *table= tl->table; > + > + if (!table || (!table->s->period.name && !table->versioned())) > + return 0; > + > +#ifndef NO_EMBEDDED_ACCESS_CHECKS > + check_access(thd, SELECT_ACL, db_name->str, > + &tl->grant.privilege, 0, 0, MY_TEST(tl->schema_table)); > + if (is_temporary_table(tl)) > + { > + tl->grant.privilege|= TMP_TABLE_ACLS; > + } > +#endif > + int err= 0; > + if (table->versioned()) > + err= store_schema_period_record(thd, tl, schema_table, db_name, table_name, > + table->s->vers); > + if (!err && table->s->period.name) > + err= store_schema_period_record(thd, tl, schema_table, db_name, table_name, > + table->s->period); > + > + return err; > +} > > -static int get_schema_column_record(THD *thd, TABLE_LIST *tables, > - TABLE *table, bool res, > - const LEX_CSTRING *db_name, > - const LEX_CSTRING *table_name) > +static > +int get_schema_column_record(THD *thd, TABLE_LIST *tables, > + TABLE *table, //!< @ref Show::columns_fields_info > + bool res, > + const LEX_CSTRING *db_name, > + const LEX_CSTRING *table_name) > { > LEX *lex= thd->lex; > const char *wild= lex->wild ? lex->wild->ptr() : NullS; > @@ -7513,30 +7592,41 @@ static int get_schema_triggers_record(THD *thd, TABLE_LIST *tables, > DBUG_RETURN(0); > } > > +static int > +store_key_column_usage(TABLE *table, const LEX_CSTRING &db_name, > + const LEX_CSTRING &table_name, > + const LEX_CSTRING &key_name, > + const LEX_CSTRING &col_name) > +{ > + CHARSET_INFO *cs= system_charset_info; > + static const LEX_CSTRING def{STRING_WITH_LEN("def")}; > + const LEX_CSTRING *values[] { &def, &db_name, &key_name, > + &def, &db_name, &table_name, &col_name }; > + int err = 0; > + for (uint i = 0; i < array_elements(values) && !err; i++) > + table->field[i]->store(values[i], cs); you forgot `err=` here :) but as I wrote above, it's not needed here anyway > + return 0; > +} > > static void > store_key_column_usage(TABLE *table, const LEX_CSTRING *db_name, > const LEX_CSTRING *table_name, const char *key_name, > - size_t key_len, const char *con_type, size_t con_len, > + size_t key_len, const char *col_name, size_t col_len, > longlong idx) > { > - CHARSET_INFO *cs= system_charset_info; > - table->field[0]->store(STRING_WITH_LEN("def"), cs); > - table->field[1]->store(db_name->str, db_name->length, cs); > - table->field[2]->store(key_name, key_len, cs); > - table->field[3]->store(STRING_WITH_LEN("def"), cs); > - table->field[4]->store(db_name->str, db_name->length, cs); > - table->field[5]->store(table_name->str, table_name->length, cs); > - table->field[6]->store(con_type, con_len, cs); > + store_key_column_usage(table, *db_name, *table_name, {key_name, key_len}, > + {col_name, col_len}); pretty, but better, please, put it back. You can use the old store_key_column_usage from your get_schema_key_period_usage_record() just the same. There's no need to have two store_key_column_usage() functions. > table->field[7]->store((longlong) idx, TRUE); > } > > > -static int get_schema_key_column_usage_record(THD *thd, > - TABLE_LIST *tables, > - TABLE *table, bool res, > - const LEX_CSTRING *db_name, > - const LEX_CSTRING *table_name) > +static int > +get_schema_key_column_usage_record(THD *thd, TABLE_LIST *tables, > + TABLE *table, > + //!< @ref Show::key_column_usage_fields_info > + bool res, > + const LEX_CSTRING *db_name, > + const LEX_CSTRING *table_name) > { > DBUG_ENTER("get_schema_key_column_usage_record"); > if (res) > @@ -9703,6 +9824,18 @@ ST_FIELD_INFO key_column_usage_fields_info[]= > CEnd() > }; > > +ST_FIELD_INFO key_period_usage_fields_info[]= > +{ > + Column("CONSTRAINT_CATALOG", Catalog(), NOT_NULL, OPEN_FULL_TABLE), > + Column("CONSTRAINT_SCHEMA", Name(), NOT_NULL, OPEN_FULL_TABLE), > + Column("CONSTRAINT_NAME", Name(), NOT_NULL, OPEN_FULL_TABLE), > + Column("TABLE_CATALOG", Catalog(), NOT_NULL, OPEN_FULL_TABLE), > + Column("TABLE_SCHEMA", Name(), NOT_NULL, OPEN_FULL_TABLE), > + Column("TABLE_NAME", Name(), NOT_NULL, OPEN_FULL_TABLE), > + Column("PERIOD_NAME", Name(), NOT_NULL, OPEN_FULL_TABLE), Cannot they all be OPEN_FRM_ONLY? > + CEnd() > +}; > + > > ST_FIELD_INFO table_names_fields_info[]= > { > @@ -10208,6 +10358,8 @@ ST_SCHEMA_TABLE schema_tables[]= > {0, 0, 0, 0, 0, 0, 0, 0, 0, 0} > }; > > +static_assert(array_elements(schema_tables) == SCH_ENUM_SIZE + 1, > + "Update enum_schema_tables as well."); +1 > > int initialize_schema_table(st_plugin_int *plugin) > { Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 6

Re: 23b15d24eb3: cleanup: add Field::store_yesno
by Sergei Golubchik 31 Aug '23

31 Aug '23

Hi, Nikita, On Aug 31, Nikita Malyavin wrote: > revision-id: 23b15d24eb3 (mariadb-11.0.1-227-g23b15d24eb3) > parent(s): cb37c99dd87 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-08-30 13:18:52 +0400 > message: > > cleanup: add Field::store_yesno I agree, it's a good idea to have a helper for storing YES/NO in the field. But I think Field class itself is a wrong place for it. YES/NO is exclusively INFORMATION_SCHEMA thing and it feels kind of wrong to have it in the base class. I'd do a separate function, like store_yes_no(). Or something. Also, note that you missed few places where it's needed, like pos=(uchar*) ((field->flags & NOT_NULL_FLAG) ? "NO" : "YES"); table->field[6]->store((const char*) pos, strlen((const char*) pos), cs); or if (updatable_view) table->field[5]->store(STRING_WITH_LEN("YES"), cs); else table->field[5]->store(STRING_WITH_LEN("NO"), cs); both in sql_show.cc. There are more. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: Client-server protocol improvement proposal: Connection redirection
by Otto Kekäläinen 28 Aug '23

28 Aug '23

Hi Pali! On Fri, 18 Aug 2023 at 11:47, Pali <pali(a)cpan.org> wrote: > CCing choroba (for Perl's DBD::MariaDB) and replying to developers@ ML. > > I have read proposal and the first thing which worries me is how the > proposal handles security aspect of the whole solution. > ... > If TLS is used then any protocol redirection information must happen > inside of the TLS tunnel. Thanks for the comments! Actually Daniel's submission has already this covered and the redirect works only if the connection is TLS protected: * https://github.com/MariaDB/server/pull/2681, server-side implementation * https://github.com/mariadb-corporation/mariadb-connector-c/pull/226, MariaDB Connector/C implementation

1 0

Re: [MariaDB commits] [PATCH 4/4] MDEV-31273: Precompute binlog checksums
by Marko Mäkelä 25 Aug '23

25 Aug '23

On Fri, Aug 25, 2023 at 10:16 AM Kristian Nielsen via commits <commits(a)lists.mariadb.org> wrote: > > Compute binlog checksums (when enabled) already when writing events > into the statement or transaction caches, where before it was done > when the caches are copied to the real binlog file. This moves the > checksum computation outside of holding LOCK_log, improving > scalabitily. > > At stmt/trx cache write time, the final end_log_pos values are not > known, so with this patch these will be set to 0. [...] > Checksums cannot be pre-computed when binlog encryption is enabled, as > encryption relies on correct end_log_pos to provide part of the > nonce/IV. Checksum pre-computation is also disabled for WSREP/Galera. Have you thought about a format change that would address these issues? In MDEV-14425 https://github.com/MariaDB/server/commit/685d958e38b825ad9829be311f26729ccc… I redesigned the InnoDB log file format in a way that allows checksums to be computed before the final position of the records (the log sequence number of LSN) is known. For encryption, an additional 64-bit nonce will be written; this currently is the LSN at the time the encryption was scheduled. Tablespace identifiers, page numbers and file names are not encrypted, because these were never fully encrypted in innodb_encrypt_tables=ON either. This allows the metadata to be used as initialization vectors, and also avoids the need for backup tools to know any encryption keys. Thanks to MDEV-27774, not only InnoDB computes checksums while not holding any mutex, but multiple threads can copy their mini-transactions to the shared log_sys.buf while holding a shared lock_sys.latch. An exclusive latch will only be needed during a log checkpoint when we need to guarantee that all changes up to a particular LSN have been written to the log buffer. This would not be possible without the format change that was implemented in MDEV-14425. > +/* Convenience macros since ulong is 32 or 64 bit depending on platform. */ > +#if SIZEOF_LONG == 4 > +#define my_atomic_storeul(P, D) my_atomic_store32((int32 volatile *)(P), (D)) In all currently supported MariaDB Server versions, C++11 is available. Is there a particular reason why you would not use std::atomic<size_t> here? Marko -- Marko Mäkelä, Lead Developer InnoDB MariaDB plc

2 3

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