- developers - lists.mariadb.org

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

04 Aug '23

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

2 1

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

04 Aug '23

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

2 3

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

03 Aug '23

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

1 0

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

03 Aug '23

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

2 1

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

03 Aug '23

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

8 19

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