- developers - lists.mariadb.org

MariaDB topic on GitHub
by Otto Kekäläinen 19 Mar '24

19 Mar '24

Hi! I was browsing the GitHub topic 'mariadb' at https://github.com/topics/mariadb and to my surprise MariaDB itself isn't in that list at all. This seems to be due to https://github.com/MariaDB/server not having any topics defined at all. I recommend you add in the GitHub settings of https://github.com/MariaDB/server at least the topic 'mariadb', and while at it, also add link to mariadb.org. More ideas on how to improve the look and features of the GitHub page can be found by browsing projects like https://github.com/pulsar-edit and https://github.com/prisma which have all their GitHub account metadata polished. - Otto

1 0

Re: [PATCH] MDEV-33551: Semi-sync Wait Point AFTER_COMMIT Slow on Workloads with Heavy Concurrency
by Kristian Nielsen 18 Mar '24

18 Mar '24

> From: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > > When using semi-sync replication with > rpl_semi_sync_master_wait_point=AFTER_COMMIT, the performance of the > primary can significantly reduce compared to AFTER_SYNC's > performance for workloads with many concurrent users executing > transactions. This is because all connections on the primary share Hi Brandon, Here is my second round review of the MDEV-33551 patch. I think these comments of mine from first round still need to be addressed: > > A bit of care must be taken about lifetime of Tranx_node and THD > > respectively (in case one goes away but not the other). But the whole > > > +void Repl_semi_sync_master::await_all_slave_replies() > > > + wait_result= cond_timewait(front.thd, &abstime); > > > > I'm wondering, is it safe here to wait on front.thd->COND_wakeup_ready? > > Isn't there a possibility that this might go away during the wait? I tried a simple testcase (attached) which disconnects the session thread while await_all_slave_replies() is running during shutdown. As I expected, I could see the COND_wakeup_ready of the session thread being de-allocated while await_all_slave_replies() is still waking up from that condition. That cannot be correct. Similarly, it seems clearly possible that a session thread is killed and exits while its entry is still present in the m_active_tranxs list, which will similarly lead to signalling a de-allocated COND_wakeup_ready. When a thread aborts the wait before the entry is removed, I think it should simply look up its entry (can use the m_trx_htb hash table, or even just iterate the list) and set its THD to NULL (or remove the entry, whatever is simpler). For the await_all_slave_replies() problem, I think the whole wait-at-shutdown is much too complex as it is now. The whole is_thd_awaiting_semisync_ack(), n_threads_awaiting_ack, special-case not killing threads waiting for semi-sync-ack, and now introducing unmark_thd_awaiting_ack. And it still has the bug waiting on a COND_wakeup_ready of another THD that can disappear during the wait. I think all of that should just go away, the wait in await_all_slave_replies() should be done much simpler with a dedicated condition variable to wait for. Eg. await_all_slave_replies() just does a mysql_cond_wait(COND_binlog_send) under LOCK_binlog until the last binlog position is ack'ed, and this condition is then signalled from report_reply_binlog() (either always, or await_all_slave_replies() can set a flag to request signal). Or have the shutdown thread enqueue its THD at the end of the m_active_tranxs list. Let's avoid all this complex interaction between SHUTDOWN WAIT FOR ALL SLAVES and session threads. BTW, COND_binlog_send is currently unused in your patch. Below detailed comments on individual parts of the patch: > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -1750,18 +1750,12 @@ static void close_connections(void) > - if (shutdown_wait_for_slaves && repl_semisync_master.get_master_enabled()) > + if (shutdown_wait_for_slaves && repl_semisync_master.get_master_enabled() && > + repl_semisync_master.sync_get_master_wait_sessions()) Do you need to check sync_get_master_wait_sessions() > 0 here? Why not wait unconditionally for the last binlog position sent? > --- a/sql/semisync_master.cc > +++ b/sql/semisync_master.cc > @@ -68,6 +68,14 @@ static ulonglong timespec_to_usec(const struct timespec *ts) > return (ulonglong) ts->tv_sec * TIME_MILLION + ts->tv_nsec / TIME_THOUSAND; > } > > +int signal_waiting_transaction(THD *waiting_thd, const char *binlog_file, > + my_off_t binlog_pos) > +{ > + if (waiting_thd->is_awaiting_semisync_ack) > + mysql_cond_signal(&waiting_thd->COND_wakeup_ready); The check on is_awaiting_semi_sync_ack seems wrong here. Normally, the THD will be waiting, otherwise it would not be in the list in the first place, so faster with an occasional redundant signal than a conditional always. And if the THD is not waiting, it's not safe to access it anyway. Instead, as mentioned above, have a check for thd != NULL here, and let a non-waiting thread clear the THD in its entry before it releases the LOCK_binlog. > -int Repl_semi_sync_master::cond_timewait(struct timespec *wait_time) > +int Repl_semi_sync_master::cond_timewait(THD *thd, struct timespec *_wait_time) > + if (_wait_time == NULL) > + { > + create_timeout(&gen_wait_time, NULL); > + real_wait_time= &gen_wait_time; > + } > + else > + { > + real_wait_time= _wait_time; > + } Don't pass in booleans that are always hard-coded true or false in the caller. Instead create two different functions, one that takes a time_spec, and one that does not (the latter creating it and calling the former). > @@ -533,7 +559,8 @@ void Repl_semi_sync_master::remove_slave() > Signal transactions waiting in commit_trx() that they do not have to > wait anymore. > */ > - cond_broadcast(); > + m_active_tranxs->clear_active_tranx_nodes(NULL, NULL, > + signal_waiting_transaction); This does not compile on my machine (the second NULL is passed to my_off_t position), is that a last-minute change that was never tested? Or NULL is defined differently on your machine, so it doesn't fail to compile? BTW, question about the old code: Why didn't the old code clear the m_active_tranxs list? Was it a bug? Or was it cleared elsewhere before (and if so where)? > -void Repl_semi_sync_master::await_slave_reply() > +void Repl_semi_sync_master::await_all_slave_replies() > + The lock is taken per iteration rather than over the whole loop to allow > + more opportunity for the user threads to handle their ACKs. That sounds like a mis-understanding. The lock is always released by mysql_cond_timedwait() in each iteration of the loop, no need to release/take it again for just a couple instructions. > + while (TRUE) > + { > + lock(); > + if (m_active_tranxs->is_empty() || !get_master_enabled() || !is_on()) > + { > + unlock(); > + break; > + } > + wait_result= m_active_tranxs->for_front( > + [](THD *thd, const char *binlog_file, my_off_t binlog_pos) -> int { > + return (thd->is_awaiting_semisync_ack) > + ? repl_semisync_master.cond_timewait(thd, NULL) > + : 0; > + }); As I wrote at the top, it is not safe to wait on the COND_wakeup_ready of another THD, since that THD might go away during the wait. Instead, let's do a specific wait for the end position here, either on COND_binlog or by enqueing an extra THD at the end. (And btw, in your code, why wait for the _front_ of the list and not the _rear_?) > diff --git a/sql/semisync_master.h b/sql/semisync_master.h > index 99f46869354..1eb690b572b 100644 > --- a/sql/semisync_master.h > +++ b/sql/semisync_master.h > > + * The pre_delete_hook parameter is a function pointer that will be invoked > + * for each Active_tranx node, in order, from m_trx_front to log_file_name, > + * e.g. to signal their wakeup condition. Repl_semi_sync_binlog::LOCK_binlog > + * is held while this is invoked. "from m_trx_front to log_file_name", I don't understand? (Typo s/log_file_name/m_trx_rear/ ?) > +/* > + Element in Repl_semi_sync_master::wait_queue to preserve the state of a > + transaction waiting for an ACK. > +*/ > +typedef struct _semisync_wait_trx { > + const char *binlog_name; > + my_off_t binlog_pos; > + THD *thd; > +} semisync_wait_trx_t; > + This is not longer used (should be removed). > * Input: (the transaction events' ending binlog position) > * trx_wait_binlog_name - (IN) ending position's file name > * trx_wait_binlog_pos - (IN) ending position's file offset > + * unmark_thd_awaiting_ack - (IN) Whether or not to unmark > + * thd->is_awaiting_semisync_ack after receiving > + * an ACK from the replica. If using wait_point > + * AFTER_SYNC with binlog_group_commit, only the > + * last transaction written to binlog in the > + * group should negate the boolean, because the > + * same thread (i.e. leader thread) will wait for > + * all transaction ACKs. Negating it too early > + * would break SHUTDOWN WAIT FOR ALL SLAVES, as > + * that is the condition tested to bypass killing > + * threads in phase 1. In all other situations, > + * the boolean should be negated immediately. That seems unnecessarily convoluted. I think the real problem here is that write_tranx_in_binlog() sets the is_awaiting_semi_sync_ack flag on the wrong THD, on the waiter_thd instead of the trx_thd. But once we fix the await_all_slave_replies(), do we even need the is_awaiting_semi_sync_ack flag any more? If so, why? If not, then this point becomes moot anyway. - Kristian. --source include/have_innodb.inc --source include/have_debug.inc --source include/have_binlog_format_mixed.inc --source include/master-slave.inc --connection master CREATE TABLE t1 (a INT PRIMARY KEY, b INT) ENGINE=InnoDB; INSERT INTO t1 VALUES (1,0); --sync_slave_with_master source include/stop_slave.inc; SET @old_semi_enabled= @@GLOBAL.rpl_semi_sync_slave_enabled; SET GLOBAL rpl_semi_sync_slave_enabled = 1; --source include/start_slave.inc --connection master SET GLOBAL rpl_semi_sync_master_enabled= 1; SET GLOBAL rpl_semi_sync_master_timeout= 5000; let $status_var= Rpl_semi_sync_master_clients; let $status_var_value= 1; source include/wait_for_status_var.inc; show status like 'Rpl_semi_sync_master_status'; show status like 'Rpl_semi_sync_master_clients'; --connection slave SET @old_dbug= @@GLOBAL.debug_dbug; SET GLOBAL debug_dbug="+d,simulate_delay_semisync_slave_reply"; --connect(con1, localhost, root,,) --connect(con2, localhost, root,,) --connection con1 send INSERT INTO t1 VALUES (2,0); --sleep 0.3 --disconnect con1 --connection default --write_file $MYSQLTEST_VARDIR/tmp/mysqld.1.expect wait EOF #let $status_var= Rpl_semi_sync_master_wait_sessions; #let $status_var_value= 1; #source include/wait_for_status_var.inc; --sleep 0.1 --connection con2 SHUTDOWN WAIT FOR ALL SLAVES; --disconnect con2 --connection default --source include/wait_until_disconnected.inc --connection master --source include/wait_until_disconnected.inc --connection server_1 --source include/wait_until_disconnected.inc --connection default --append_file $MYSQLTEST_VARDIR/tmp/mysqld.1.expect restart EOF --enable_reconnect --source include/wait_until_connected_again.inc --connection master --enable_reconnect --source include/wait_until_connected_again.inc --connection server_1 --enable_reconnect --source include/wait_until_connected_again.inc --save_master_pos --connection slave SET GLOBAL debug_dbug= @old_dbug; SET GLOBAL rpl_semi_sync_slave_enabled= @old_semi_enabled; --source include/stop_slave.inc --source include/start_slave.inc --sync_with_master --connection default DROP TABLE t1; --source include/rpl_end.inc

2 3

Re: e6cacca0b39: MDEV-23729 MDEV-32218 INFORMATION_SCHEMA table for user data
by Sergei Golubchik 15 Mar '24

15 Mar '24

Hi, Nikita, no test for FLUSH PRIVILEGES no test for a user with manually expired password doesn't compile in embedded hostname 'neo' is replaced, but not everywhere I'll try to fix it myself, but no guarantees On Mar 15, Nikita Malyavin wrote: > revision-id: e6cacca0b39 (mariadb-11.4.1-10-ge6cacca0b39) > parent(s): 929c2e06aae > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2024-03-15 15:32:45 +0100 > message: > > MDEV-23729 MDEV-32218 INFORMATION_SCHEMA table for user data > > * A new table INFORMATION_SCHEMA.USERS is introduced. > * It stores auxiliary user data > * An unprivileged user can access their own data, and that is the main > difference with what mysql.global_priv provides > * The fields are currently: USER, PASSWORD_ERRORS, PASSWORD_EXPIRATION_TIME > * If password_errors is ignored for the user, PASSWORD_ERRORS is NULL > * PASSWORD_EXPIRATION_TIME is a timestamp with exact point in time, calculated > from password_last_changed and password_lifetime (i.e. days) stored for the user > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 14450a5a610..a87b04f3d39 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc ... > +int fill_users_schema_table(THD *thd, TABLE_LIST *tables, COND *cond) > +{ > + int res= 0; > +#ifndef NO_EMBEDDED_ACCESS_CHECKS > + bool see_whole_table= check_access(thd, SELECT_ACL, "mysql", NULL, NULL, > + true, true) == 0; > + TABLE *table= tables->table; > + > + if (!see_whole_table) > + { > + mysql_mutex_lock(&acl_cache->lock); > + ACL_USER *cur_user= find_user_exact(thd->security_ctx->priv_host, > + thd->security_ctx->priv_user); > + if (!cur_user) > + { > + mysql_mutex_unlock(&acl_cache->lock); > + my_error(ER_INVALID_CURRENT_USER, MYF(0)); Hmm. Questionable. I'd just return an empty result set here, I_S tables normally don't throw errors. But this is an exceptional situation and I cannot say that an error is wrong here, let's keep it your way. > + return 1; > + } > + > + res= fill_users_schema_record(thd, table, cur_user); > + mysql_mutex_unlock(&acl_cache->lock); > + return res; > + } > + > + mysql_mutex_lock(&acl_cache->lock); > + for (size_t i= 0; res == 0 && i < acl_users.elements; i++) > + { > + ACL_USER *user= dynamic_element(&acl_users, i, ACL_USER*); > + res= fill_users_schema_record(thd, table, user); > + } > + mysql_mutex_unlock(&acl_cache->lock); > +#endif > + return res; > +} > + Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: [PATCH 0/2] Suggestion for smaller fix to XA parallel replication performance, MDEV-31949
by Kristian Nielsen 13 Mar '24

13 Mar '24

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: > which makes a worker with dependent pieces of job be *effectively* placed at the > tail of the available worker ('fifo') queue. Indeed. And this BTW is why a simple sliding window of size 2*workers is not enough, because in principle a worker can sit in the fifo for many transactions without being scheduled, if there are many dependency restrictions. I coded the thing with "generations" as a quick way to handle this and have a way to clean out old dependency information. This could of course be improved in many ways. Maybe remember the two last XID for every worker and have a hash table mapping each XID to its worker, or something. But this is just a PoC. > No you are not. Thanks for the catch! To excuse myself it was supposed > to be a poc that I committed hastedly. Yes yes, no problem, I understand, it's good to do poc. I was just momentarily confused, but the intent is clear, thanks. - Kristian.

1 0

Re: a473db9e4fe: MDEV-25829 Change default collation to utf8mb4_1400_ai_ci
by Sergei Golubchik 13 Mar '24

13 Mar '24

Hi, Alexander, On Mar 08, Alexander Barkov wrote: > commit a473db9e4fe > Author: Alexander Barkov <bar(a)mariadb.com> > Date: Thu Nov 2 14:16:09 2023 +0400 > > MDEV-25829 Change default collation to utf8mb4_1400_ai_ci "utf8mb4_uca1400_ai_ci". It's still not too late to rename the MDEV and change commits to match. I'm not reviewing InnoDB changes, I hope Marko did > > Step#3 The main patch > > diff --git a/mysql-test/include/ctype_utf8mb4.inc b/mysql-test/include/ctype_utf8mb4.inc > index 436b0f2782f..73011cef81d 100644 > --- a/mysql-test/include/ctype_utf8mb4.inc > +++ b/mysql-test/include/ctype_utf8mb4.inc > @@ -58,11 +58,11 @@ select CONVERT(_koi8r' > # "a\0" < "a" > # "a\0" < "a " > > -SELECT 'a' = 'a '; > -SELECT 'a\0' < 'a'; > -SELECT 'a\0' < 'a '; > -SELECT 'a\t' < 'a'; > -SELECT 'a\t' < 'a '; > +SELECT 'a' = 'a ' collate utf8mb4_general_ci; > +SELECT 'a\0' < 'a' collate utf8mb4_general_ci; > +SELECT 'a\0' < 'a ' collate utf8mb4_general_ci; > +SELECT 'a\t' < 'a' collate utf8mb4_general_ci; > +SELECT 'a\t' < 'a ' collate utf8mb4_general_ci; 1. what will happen in utf8mb4_uca1400_ai_ci? 2. if you specifically want this file to test utf8mb4_general_ci, you can do SET NAMES, cannot you? Once for the whole file. Or set character_set_collations > > # > # The same for binary collation > diff --git a/mysql-test/main/ctype_euckr.result b/mysql-test/main/ctype_euckr.result > index 9e030f9cc6d..8de90f89a26 100644 > --- a/mysql-test/main/ctype_euckr.result > +++ b/mysql-test/main/ctype_euckr.result > @@ -2244,7 +2244,7 @@ FE80 > DELETE FROM t2 WHERE a='?'; > ALTER TABLE t2 ADD u VARCHAR(1) CHARACTER SET utf8, ADD a2 VARCHAR(1) CHARACTER SET euckr; > UPDATE IGNORE t2 SET u=a, a2=u; > -SELECT s as unassigned_code FROM t2 WHERE u='?'; > +SELECT s as unassigned_code FROM t2 WHERE u=binary'?'; why? > unassigned_code > A2E8 > A2E9 > diff --git a/mysql-test/main/ctype_like_range.result b/mysql-test/main/ctype_like_range.result > index e5e5e61126b..fa694b0c250 100644 > --- a/mysql-test/main/ctype_like_range.result > +++ b/mysql-test/main/ctype_like_range.result > @@ -284,7 +284,7 @@ id name val > 32 mn 63616161616161616161616161616161 > 32 mx 63616161616161616161616161616161 > 32 sp -------------------------------- > -ALTER TABLE t1 MODIFY a VARCHAR(32) CHARACTER SET utf8; > +ALTER TABLE t1 MODIFY a VARCHAR(32) CHARACTER SET utf8 COLLATE utf8_general_ci; do you have tests for ctype_like_range with utf8mb3_uca1400_ai_ci ? > INSERT INTO t1 (a) VALUES (_ucs2 0x0425),(_ucs2 0x045F); > INSERT INTO t1 (a) VALUES (_ucs2 0x2525),(_ucs2 0x5F5F); > SELECT * FROM v1; > diff --git a/mysql-test/main/ctype_utf8.test b/mysql-test/main/ctype_utf8.test > index a875fe51f3a..2f56d10bccf 100644 > --- a/mysql-test/main/ctype_utf8.test > +++ b/mysql-test/main/ctype_utf8.test > @@ -24,7 +24,7 @@ drop database if exists mysqltest; > --disable_warnings > drop table if exists t1,t2; > --enable_warnings > -set names utf8; > +set names utf8 collate utf8_general_ci; why not to change the test to work with utf8mb3_uca1400_ai_ci? > > select left(_utf8 0xD0B0D0B1D0B2,1); > select right(_utf8 0xD0B0D0B2D0B2,1); > diff --git a/mysql-test/main/dyncol.test b/mysql-test/main/dyncol.test > index 493e9b3842d..ba302861e03 100644 > --- a/mysql-test/main/dyncol.test > +++ b/mysql-test/main/dyncol.test > @@ -10,10 +10,10 @@ > --echo # column create > --echo # > select hex(COLUMN_CREATE(1, NULL AS char character set utf8)); > -select hex(COLUMN_CREATE(1, "afaf" AS char character set utf8)); > -select hex(COLUMN_CREATE(1, 1212 AS char character set utf8)); > -select hex(COLUMN_CREATE(1, 12.12 AS char character set utf8)); > -select hex(COLUMN_CREATE(1, 99999999999999999999999999999 AS char character set utf8)); > +select hex(COLUMN_CREATE(1, "afaf" AS char character set utf8 collate utf8_general_ci)); > +select hex(COLUMN_CREATE(1, 1212 AS char character set utf8 collate utf8_general_ci)); > +select hex(COLUMN_CREATE(1, 12.12 AS char character set utf8 collate utf8_general_ci)); > +select hex(COLUMN_CREATE(1, 99999999999999999999999999999 AS char character set utf8 collate utf8_general_ci)); why? > select hex(COLUMN_CREATE(1, NULL AS unsigned int)); > select hex(COLUMN_CREATE(1, 1212 AS unsigned int)); > select hex(COLUMN_CREATE(1, 7 AS unsigned int)); > diff --git a/mysql-test/main/func_json.result b/mysql-test/main/func_json.result > index ab35822a6ce..fce2caa2842 100644 > --- a/mysql-test/main/func_json.result > +++ b/mysql-test/main/func_json.result > @@ -414,6 +414,13 @@ select json_object('foo', json_unquote(json_object('bar', c)),'qux', c) as fld f > fld > {"foo": "{\"bar\": \"abc\"}", "qux": "abc"} > {"foo": "{\"bar\": \"def\"}", "qux": "def"} > +create table t2 as select json_object('foo', json_unquote(json_object('bar', c)),'qux', c) as fld from t1 limit 0; > +show create table t2; > +Table Create Table > +t2 CREATE TABLE `t2` ( > + `fld` varchar(39) DEFAULT NULL > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 COLLATE=latin1_swedish_ci > +drop table t2; why do you test here? > drop table t1; > select json_object("a", json_object("b", "abcd")); > json_object("a", json_object("b", "abcd")) > diff --git a/mysql-test/main/func_regexp_pcre.test b/mysql-test/main/func_regexp_pcre.test > index 77f5af6e0ff..7cf9e8007de 100644 > --- a/mysql-test/main/func_regexp_pcre.test > +++ b/mysql-test/main/func_regexp_pcre.test > @@ -23,17 +23,17 @@ SELECT 'Ã ' RLIKE '\\x{00C0}' COLLATE utf8_bin; > SELECT 'Ã' RLIKE '\\x{00C0}' COLLATE utf8_bin; > > # Checking how (?i) and (?-i) affect case sensitivity > -CREATE TABLE t1 (s VARCHAR(10) CHARACTER SET utf8); > +CREATE TABLE t1 (s VARCHAR(10) CHARACTER SET utf8 COLLATE utf8_general_ci); why? > INSERT INTO t1 VALUES ('a'),('A'); > -CREATE TABLE t2 (p VARCHAR(10) CHARACTER SET utf8); > +CREATE TABLE t2 (p VARCHAR(10) CHARACTER SET utf8 COLLATE utf8_general_ci); > INSERT INTO t2 VALUES ('a'),('(?i)a'),('(?-i)a'),('A'),('(?i)A'),('(?-i)A'); > SELECT s,p,s RLIKE p, s COLLATE utf8_bin RLIKE p FROM t1,t2 ORDER BY BINARY s, BINARY p; > DROP TABLE t1,t2; > > > # Checking Unicode character classes > -CREATE TABLE t1 (ch VARCHAR(22)) CHARACTER SET utf8; > -CREATE TABLE t2 (class VARCHAR(32)) CHARACTER SET utf8; > +CREATE TABLE t1 (ch VARCHAR(22)) CHARACTER SET utf8 COLLATE utf8_general_ci; > +CREATE TABLE t2 (class VARCHAR(32)) CHARACTER SET utf8 COLLATE utf8_general_ci; and here > INSERT INTO t1 VALUES ('Ð¯'),('Î£'),('A'),('Ã'); > INSERT INTO t1 VALUES ('Ñ'),('Ï'),('a'),('Ã '); > INSERT INTO t1 VALUES ('ã'),('ê°·'),('à¶´'); > diff --git a/mysql-test/main/mrr_icp_extra.result b/mysql-test/main/mrr_icp_extra.result > index 8f6ee88acc6..d7cde103288 100644 > --- a/mysql-test/main/mrr_icp_extra.result > +++ b/mysql-test/main/mrr_icp_extra.result > @@ -754,7 +754,7 @@ t1 CREATE TABLE `t1` ( > KEY `t` (`t`(5)) > ) ENGINE=MyISAM DEFAULT CHARSET=latin1 COLLATE=latin1_swedish_ci > drop table t1; > -create table t1 (v char(10) character set utf8); > +create table t1 (v char(10) character set utf8 collate utf8_general_ci); why here? > show create table t1; > Table Create Table > t1 CREATE TABLE `t1` ( > diff --git a/mysql-test/main/myisam.test b/mysql-test/main/myisam.test > index 1a20f97a54f..4a283737122 100644 > --- a/mysql-test/main/myisam.test > +++ b/mysql-test/main/myisam.test > @@ -1039,7 +1039,7 @@ create table t1 (v varchar(65536)); > show create table t1; > drop table t1; > set statement sql_mode = 'NO_ENGINE_SUBSTITUTION' for > -create table t1 (v varchar(65530) character set utf8); > +create table t1 (v varchar(65530) character set utf8 collate utf8_general_ci); why? collation doesn't matter here, does it? > show create table t1; > drop table t1; > > diff --git a/mysql-test/main/order_by.result b/mysql-test/main/order_by.result > index 274f29e34dc..85059fb3370 100644 > --- a/mysql-test/main/order_by.result > +++ b/mysql-test/main/order_by.result > @@ -4142,7 +4142,7 @@ drop table t1; > # > # MDEV-21922: Allow packing addon fields even if they don't honour max_length_for_sort_data > # > -create table t1 (a varchar(200) character set utf8, b int); > +create table t1 (a varchar(200) character set utf8 collate utf8_general_ci, b int); why? I think I see too many forced utf8_general_ci in tests and it's difficult to believe that the collation actually matters in all these cases. > insert into t1 select seq, seq from seq_1_to_10; > select * from t1 order by a; > a b > diff --git a/sql/item_jsonfunc.cc b/sql/item_jsonfunc.cc > index 508ea9f644e..5731ad1f04b 100644 > --- a/sql/item_jsonfunc.cc > +++ b/sql/item_jsonfunc.cc > @@ -842,7 +842,7 @@ String *Item_func_json_quote::val_str(String *str) > bool Item_func_json_unquote::fix_length_and_dec(THD *thd) > { > collation.set(&my_charset_utf8mb3_general_ci, > - DERIVATION_COERCIBLE, MY_REPERTOIRE_ASCII); > + DERIVATION_CAST, MY_REPERTOIRE_ASCII); why? I guess that's why you have json_unquote test above. > max_length= args[0]->max_length; > set_maybe_null(); > return FALSE; Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: b657bc75f90: MDEV-25829 Change default collation to utf8mb4_1400_ai_ci
by Sergei Golubchik 13 Mar '24

13 Mar '24

Hi, Alexander, On Mar 08, Alexander Barkov wrote: > revision-id: b657bc75f90 (mariadb-11.0.1-262-gb657bc75f90) > parent(s): 8e5f41b3453 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2023-11-15 10:58:03 +0400 > message: > > MDEV-25829 Change default collation to utf8mb4_1400_ai_ci > > Step#2 - Adding a new collation derivation level for CAST and CONVERT. > > Now character string cast functions: > - CAST(string_expr AS CHAR) > - CONVERT(expr USING charset_name) > > have a new collation derivation level between: > > - string literals > - utf8 metadata functions, e.g. user() and database() > > Before the change these cast functions had collation derivation equal > to table columns, which caused more illegal mix of collation conflicts. > > Note, binary string cast functions: > - CAST(string_expr AS BINARY) > - CONVERT(expr USING binary) > did not change their collation derivation. Why not? Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: 8e5f41b3453: MDEV-25829 Change default collation to utf8mb4_1400_ai_ci
by Sergei Golubchik 13 Mar '24

13 Mar '24

Hi, Alexander, On Mar 08, Alexander Barkov wrote: > revision-id: 8e5f41b3453 (mariadb-11.0.1-261-g8e5f41b3453) > parent(s): 9e457cbe501 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2023-11-15 09:37:26 +0400 > message: > > MDEV-25829 Change default collation to utf8mb4_1400_ai_ci > > Step#1 - Changing collation derivation for string user variables > from EXPLICIT to COERCIBLE. from IMPLICIT, oherwise ok to push. and I still think user variable could remember the derivation. It's definitely not a part of MDEV-25829, but if you agree that it should, let's create an MDEV for that. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: [PATCH 0/2] Suggestion for smaller fix to XA parallel replication performance, MDEV-31949
by Kristian Nielsen 12 Mar '24

12 Mar '24

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: > I reviewed your branch to agree with the idea of xid conflicts > handling and its implementation. > > I however could not understand the need of the refactoring part. > In order to track the xid dependency couple functions and and > rpl_parallel_entry::maybe_active_xid > a sort of a sliding window - that the 2nd commit of your branch > introduced must be sufficient to my analysis. > Of course it needs to hold worker indexes of active xid:s. > > This observation led me to create a review branch > > origin/review__knielsen_xa_sched_minimal_fix > > [where origin git@github.com:MariaDB/server.git] > which is a somewhat light elaboration over the 2nd commit of your branch. The reason for the refactoring part is to allow to distribute the transactions as evenly as possible over the worker threads, given the scheduling constraints imposed by XA dependencies. Let's say we have 5 worker threads, and transactions T1..T15. If there are no scheduling constraints, we can schedule evenly like this: W1: T1 T6 T11 W2: T2 T7 T12 W3: T3 T8 T13 W4: T4 T9 T14 W5: T5 T10 T15 Now suppose we need to schedule T6 on the same worker as T4, and T8 on the same worker as T3. The most even way to distribute the transactions is now this: W1: T1 T7 T12 W2: T2 T9 T14 W3: T3 T8 T13 W4: T4 T6 T11 W5: T5 T10 T15 You see, we try to always have 5 consecutive transactions T_i, T_{i+1}, ..., T+{i+4} scheduled on 5 different worker threads, except when this is not possible due to dependency restrictions. But this most-even scheduling requires to change the scheduling order of threads from the original sequential 1,2,3,4,5. You see, the last transactions T11..T15 are scheduled on worker threads in order: W4, W1, W3, W2, W5. The refactoring patch introduces thread_sched_fifo to hold the current scheduling order of worker threads. Whenever we have to schedule out-of-order due to a scheduling dependency, we put the scheduled worker at the end of this fifo, to preserve even scheduling for the next N transactions. In the original scheduling code, we never scheduled workers out-of-order, so the scheduling order was always cyclic W1, W2, W3, W4, W5, W1, ..., and a simple incrementing counter was sufficient. But this is no longer sufficient when out-of-order scheduling occurs. In the example, if we use a simple counter and try to schedule on worker ((i+1) mod N) after worker (i), then we get the following uneven scheduling: W1: T1 T11 W2: T2 T12 W3: T3 T8 T13 W4: T4 T6 T9 T14 W5: T5 T7 T10 T15 Because T6 and T8 have particular scheduling requirements, they cause the scheduling to skip workers. The result is that W4 and W5 get too many transactions, while W1 and W2 get too few, and parallelism is reduced. I hope this explain the reasoning. > Could you please have a look at it? If I understand your patch correctly, it schedules using a simple incrementing counter when there are no dependency requirements, and so would suffer from the uneven scheduling in the above example. It also looks like there's a mistake in the code: > + if ((idx= check_xa_xid_dependency(&gtid_ev->xid)) < (uint32) -1) > + { > + /* > + A previously scheduled event group with the same XID might still be > + active in a worker, so schedule this event group in the same worker > + to avoid a conflict. > + */ > + } > + else > + { > + /* Record this XID now active. */ > + xid_active_generation *a= > + (xid_active_generation *)alloc_dynamic(&maybe_active_xid); > + if (!a) > + return NULL; > + ++idx; > + if (idx >= rpl_thread_max) > + idx= 0; If check_xa_xid_dependency() returns "no dependency", then idx is set to (uint32)-1 and the else {...} branch is executed. This does idx++, which increments the (uint32)-1 to 0. So it looks to me that _all_ transactions are scheduled on worker 0, or am I missing something? This is a simple mistake, it's an easy fix to not assign idx when check_xa_xid_dependency() returns "no dependency". But the code would still suffer from uneven scheduling as in the example, IIUC. So there's a good reason to introduce the thread_sched_fifo as done in my refactoring patch. This though still leaves the limitation in my minimal patch that XA PREPARE xid1 cannot group commit with XA COMMIT xid1. The impact of this will depend on how many transactions on average appear between an XA PREPARE and its associated XA COMMIT, as well as on how expensive fsync() is relative to the cost of each transaction. - Kristian.

1 0

Re: 9e5d4dfc49b: MDEV-23729 MDEV-32218 INFORMATION_SCHEMA table for user login data
by Sergei Golubchik 11 Mar '24

11 Mar '24

Hi, Nikita, On Mar 10, Nikita Malyavin wrote: > revision-id: 9e5d4dfc49b (mariadb-11.4.1-10-g9e5d4dfc49b) > parent(s): 929c2e06aae > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2024-02-29 17:24:27 +0100 > message: > > MDEV-23729 MDEV-32218 INFORMATION_SCHEMA table for user login data > > * A new table INFORMATION_SCHEMA.LOGON is introduced. There were many cases where we lack an INFORMATION_SCHEMA with all user accounts. Forcing users to select from mysql.user is a shame. Let's use the chance and create a table USERS. It doesn't need more columns now than what you've created, but we'll likely create more in the future. > * Upon idea, it stores auxiliary user data related to login/security/resources > * An unprivileged user can access their own data, and that is the main > difference with what mysql.global_priv provides exactly! > * The fields are currently: USER, WRONG_PASSWORD_ATTEMPTS, EXPIRATION_TIME I wanted to write about splitting USER into USER and HOST, but indeed it's just USER everywhere else in INFORMATION_SCHEMA (e.g. DEFINER and GRANTEE columns). And functions like USER() and CURRENT_USER() don't split either. So, you're right, let's consistently use user@host everywhere. > diff --git a/mysql-test/main/information_schema_stats.result b/mysql-test/main/information_schema_stats.result > index 352bcbab823..e38788872fa 100644 > --- a/mysql-test/main/information_schema_stats.result > +++ b/mysql-test/main/information_schema_stats.result ... > +connect(localhost,naughty_user,wrong_passwd,test,16000,/home/nik/mariadb/bld/mysql-test/var/tmp/mysqld.1.sock); here and below - you forgot to replace the path with MASTER_MYSOCK > diff --git a/mysql-test/main/information_schema_stats.test b/mysql-test/main/information_schema_stats.test > index fd5171c3fb4..dbcabe45965 100644 > --- a/mysql-test/main/information_schema_stats.test > +++ b/mysql-test/main/information_schema_stats.test > @@ -47,3 +47,69 @@ select * from information_schema.index_statistics where table_schema='test' and > select * from information_schema.table_statistics where table_schema='test' and table_name='just_a_test'; > set global userstat=@save_userstat; > --enable_ps2_protocol > + > +--echo # > +--echo # MDEV-23729 INFORMATION_SCHEMA Table info. about user locked due to > +--echo # max_password_errors > +--echo # > +--echo # MDEV-32218 message to notify end-user N-days prior the password get > +--echo # expired > +--echo # I don't see how you test "info about user locked due to max_password_errors". This is the reason for implementing this new table, it has to be tested. At least add select * from information_schema.logon where wrong_password_attemps >= @max_password_errors; and show it it's empty at first and then not empty. > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 14450a5a610..4d199bf0e61 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -12956,6 +12956,87 @@ int fill_schema_column_privileges(THD *thd, TABLE_LIST *tables, COND *cond) > #endif > } > > +namespace Show > +{ > + ST_FIELD_INFO users_fields_info[] = > + { > + Column("USER", Userhost(), NOT_NULL), > + Column("WRONG_PASSWORD_ATTEMPTS", SLonglong(), NULLABLE), @@max_password_errors is documented as "If there is more than this number of failed connect attempts due to invalid password, user will be blocked from further connections until FLUSH_PRIVILEGES" Let's use call incorrect password consistently everywhere. It could be "bad password", "wrong password", "incorrect password", "invalid password", but it should be the same everywhere. I personally think that "invalid" implies some kind if validity check, e.g. not shorter than 10 characters, not equal to the username, etc and a password that doesn't satisfy these validity rules is "invalid". Trying to login with an incorrect password is a different kind of error, a password can be valid but wrong. May be, ask Ian and/or Daniel about it? And then don't forget to update @@max_password_errors help text to match. Also, you don't have a test case for FLUSH PRIVILEGES. > + Column("EXPIRATION_TIME", SLonglong(), NULLABLE), PASSWORD_EXPIRATION_TIME, the user account itself does not expire. > + CEnd() > + }; > +}; > + > +static bool ignore_max_password_errors(const ACL_USER *acl_user); > + > +static int fill_logon_schema_record(THD *thd, TABLE * table, ACL_USER *user) > +{ > + ulonglong lifetime= user->password_lifetime < 0 > + ? default_password_lifetime > + : user->password_lifetime; > + > + bool ignore_password_errors= ignore_max_password_errors(user); why? I think it's still useful to show how many wrong password attempts were there for an account even if it doesn't get blocked. > + bool ignore_expiration_date= lifetime == 0; > + > + /* Skip user if nothing to show */ > + if (ignore_password_errors && ignore_expiration_date) > + return 0; > + > + Grantee_str grantee(user->user.str, safe_str(user->host.hostname)); > + table->field[0]->store(grantee, strlen(grantee), system_charset_info); > + if (ignore_password_errors) > + { > + table->field[1]->set_null(); > + } > + else > + { > + table->field[1]->set_notnull(); > + table->field[1]->store(user->password_errors); > + } > + if (ignore_expiration_date) > + { > + table->field[2]->set_null(); > + } > + else > + { > + table->field[2]->set_notnull(); > + table->field[2]->store(user->password_last_changed > + + user->password_lifetime * 3600 * 24, true); I think it'd be more generally useful to show password_last_changed and expiration period separately. > + } > + > + return schema_table_store_record(thd, table); > +} > + > +int fill_logon_schema_table(THD *thd, TABLE_LIST *tables, COND *cond) > +{ > + int res= 0; > +#ifndef NO_EMBEDDED_ACCESS_CHECKS > + bool see_whole_table= check_global_access(thd, PROCESS_ACL, true) == 0; I don't think PROCESS_ACL is a very logical choice here. And there's nothing better as far as I can see. May be let's just do if (check_access(thd, SELECT_ACL, "mysql", NULL, NULL, 1, 1)) ? There are many checks like that in e.g. sql_parse.cc > + > + TABLE *table= tables->table; > + > + if (!see_whole_table) > + { > + mysql_mutex_lock(&acl_cache->lock); > + ACL_USER *cur_user= find_user_exact(thd->security_ctx->priv_host, > + thd->security_ctx->priv_user); 1. cur_user can be NULL if someone dropped it while there was an active connection for this user 2. add a test for it > + > + res= fill_logon_schema_record(thd, table, cur_user); > + mysql_mutex_unlock(&acl_cache->lock); > + return res; > + } > + > + mysql_mutex_lock(&acl_cache->lock); > + for (size_t i= 0; res == 0 && i < acl_users.elements; i++) > + { > + ACL_USER *user= dynamic_element(&acl_users, i, ACL_USER*); > + res= fill_logon_schema_record(thd, table, user); > + } > + mysql_mutex_unlock(&acl_cache->lock); > +#endif > + return res; > +} > + Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 2

Backup MariaDB Databases for Suprema BioStar 2 Door Access System
by Turritopsis Dohrnii Teo En Ming 09 Mar '24

09 Mar '24

Subject: Backup MariaDB Databases for Suprema BioStar 2 Door Access System Good day from Singapore, On 7 March 2024 Thursday, when I was installing new self-signed SSL certificate for the door access system for a law firm in Singapore, I notice that Suprema BioStar 2 also uses the open source MariaDB database server. If I did not remember wrongly, there are 3 databases. However, the entire platform is a Windows environment, not Linux. How can I backup all the databases in the MariaDB database server? Thank you. Regards, Mr. Turritopsis Dohrnii Teo En Ming Targeted Individual in Singapore Blogs: https://tdtemcerts.blogspot.com https://tdtemcerts.wordpress.com GIMP also stands for Government-Induced Medical Problems.

1 0

Re: be66e975ecf: MDEV-31340 Remove MY_COLLATION_HANDLER::strcasecmp()
by Sergei Golubchik 09 Mar '24

09 Mar '24

Hi, Alexander, Ok to push, thanks for the great refactoring! I've looked at the diff of this and previous commit excluding InnoDB. On Mar 08, Alexander Barkov wrote: > revision-id: be66e975ecf (mariadb-11.0.1-282-gbe66e975ecf) > parent(s): c0c1c80346b > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2024-01-19 11:04:02 +0400 > message: > > MDEV-31340 Remove MY_COLLATION_HANDLER::strcasecmp() > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

64-bit time_t support in MariaDB?
by Otto Kekäläinen 07 Mar '24

07 Mar '24

Hello! Does MariaDB already fully support 64-bit time_t (the year 2038 problem)? MariaDB 10.11 was today uploaded to Debian experimental without any 64-bit time_t support changes, just a library name change[1] to test that it works with the Debian 64-bit time_t tooling as Debian is currently undergoing a transition to it[2]. So far MariaDB built successfully and passed the main MTR suite on 11 architectures[3]. Monty mentioned that some work is in progress, and indeed I found his commits in two branches [4,5] but looking at the commits and their link to buildbot it seems they are not passing the CI. I also found one MDEV[6] to make TIMESTAMP use the whole 32-bit unsigned range. Are there any other efforts in progress? Does MTR main suite include tests for 64-bit time support? Should MTR automatically currently fail if toolchain time is 64-bit? [1] https://salsa.debian.org/mariadb-team/mariadb-server/-/commit/063109a306a01… [2] https://lists.debian.org/debian-devel-announce/2024/02/msg00000.html [3] https://buildd.debian.org/status/package.php?p=mariadb&suite=experimental [4] https://github.com/MariaDB/server/commits/bb-11.4-timestamp/ [5] https://github.com/MariaDB/server/commits/bb-11.4-monty/ [6] https://jira.mariadb.org/browse/MDEV-32188

2 6

Re: [PATCH 2/2] MDEV-33551: Semi-sync Wait Point AFTER_COMMIT Slow on Workloads with Heavy Concurrency
by Kristian Nielsen 06 Mar '24

06 Mar '24

> commit c51636f254703602f6f6e2e4a260e607e737b9c1 (origin/10.6-MDEV-33551) > Author: Brandon Nesterenko <brandon.nesterenko(a)mariadb.com> > Date: Mon Feb 26 07:01:47 2024 -0700 > > MDEV-33551: Semi-sync Wait Point AFTER_COMMIT Slow on Workloads with Heavy Concurrency First some overall comments. The approach of waking up each thread on its own condition seems good to me. Monty had a different suggestion, which is to have only a single ack sent per group commit; this could also be a possibility, but it seems orthogonal to your fix (ie. both could make sense). Monty also had the idea to move the AFTER_COMMIT sync point into the group commit code, though that changes the semantics of AFTER_COMMIT somewhat, especially with --innodb-flush-log-at-trx-commit=3 (or non-InnoDB storage engine). So in this review I'll focus on keeping your overall approach the same. Your patch introduces a new wait_queue with the list of waiting threads. But there is already a very similar queue, m_active_tranxs. It looks to me as if the m_active_tranx could easily be extended so it can be used for this, and avoid introducing another queue. Your wait_queue is pushed in write_tranx_in_binlog() which already does m_active_tranxs->insert_tranx_node(), and it is popped in report_reply_binlog() which also does m_active_tranxs->clear_active_tranx_nodes(). So add some info to struct Tranx_node, maybe a THD * is all that is needed? A bit of care must be taken about lifetime of Tranx_node and THD respectively (in case one goes away but not the other). But the whole m_active_tranxs is already protected by a lock, and there is a hash for looking up specific position, so I think a THD that wants to abort its wait can simply clear the THD * from its Tranx_node or something like that to ensure the semisync ack will not try to wake up a THD that no longer exists. You'll need to see how this will affect the code for shutdown (await_all_slave_replies()) where it needs to wait for all acks (IIUC). But I hope we can be pragmatic here and find something simple that works. Another general thing related to this: > 3) Repl_semi_sync_master::commit_trx() no longer loops to await > its specific ACK. It waits once, and will either fail from > timeout, or receive its ACK. We still need the loop to wait. From `man pthread_cond_wait`: "When using condition variables there is always a Boolean predicate in‐ volving shared variables associated with each condition wait that is true if the thread should proceed. Spurious wakeups from the pthread_cond_timedwait() or pthread_cond_wait() functions may occur. Since the return from pthread_cond_timedwait() or pthread_cond_wait() does not imply anything about the value of this predicate, the predi‐ cate should be re-evaluated upon such return." This affects the code in the patch in some places, detailed comments below. > 2) The time when thd::is_awaiting_semi_sync_ack is set is moved > to at binlogging time, to ensure transactions which have been > binlogged and queued up to await an ACK are not killed, > and are still waited on. I don't understand this. Why shouldn't a session waiting for ACK be killable? On the contrary, such a wait can take potentially a long time, seems important that it can be killed? I don't fully understand the existing code for this, maybe the patch is correct and I'm just looking for an explanation why it is correct like this. Or maybe this is a limitation of the existing code, and your patch doesn't change it (so having the ability to kill a waiting thread could be added in a different patch)? > diff --git a/mysql-test/suite/rpl/t/rpl_semi_sync_cond_var_per_thd.test b/mysql-test/suite/rpl/t/rpl_semi_sync_cond_var_per_thd.test > index f8fa0a99d9c..ebe15a4ca32 100644 > --- a/mysql-test/suite/rpl/t/rpl_semi_sync_cond_var_per_thd.test > +++ b/mysql-test/suite/rpl/t/rpl_semi_sync_cond_var_per_thd.test > @@ -22,6 +22,7 @@ > --source include/master-slave.inc > > --connection master > +call mtr.add_suppression("Got an error reading communication packets"); Is this suppression needed? If so, why? > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > index e224871795e..b315edc091c 100644 > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -1750,18 +1750,12 @@ static void close_connections(void) > /* > If we are waiting on any ACKs, delay killing the thread until either an ACK > is received or the timeout is hit. > - while (waiting_threads-- > 0) > - repl_semisync_master.await_slave_reply(); > + repl_semisync_master.await_all_slave_replies(); Just curious here, why did you change this code? Was it just because now there is no general COND_binlog_send to wait on? Or was it fix a bug not directly related to the main issue? I'm asking because if it makes things simpler, I think it's fine to keep the COND_binlog_send and signal it in addition to the individual thread-specific condition (in fact your patch doesn't seem to remove it though it looks as if it's no longer used). On the other hand, this old code in close_connections() that simply calls await_slave_reply() N times (N = sync_get_master_wait_sessions()) looks quite wrong to me, looping for a specific number of times makes no sense. So fixing it is good in any case. > diff --git a/sql/semisync_master.cc b/sql/semisync_master.cc > index 0eaf0f0e0e2..f421e924a61 100644 > --- a/sql/semisync_master.cc > +++ b/sql/semisync_master.cc > @@ -502,16 +502,33 @@ void Repl_semi_sync_master::unlock() > > void Repl_semi_sync_master::cond_broadcast() > { > - mysql_cond_broadcast(&COND_binlog_send); > + while (!wait_queue.empty()) > + { > + semisync_wait_trx_t next_waiter= wait_queue.front(); Maybe this function should now have a better name than cond_broadcast() (wakeup_ready_tranx() or something?). But this code will probably change in any case if the wait_queue is folded into m_active_tranxs. > -int Repl_semi_sync_master::cond_timewait(struct timespec *wait_time) > +int Repl_semi_sync_master::cond_timewait(THD *thd, struct timespec *wait_time) I think this function now makes little sense, clearer just to call directly mysql_cond_wait(thd->COND_wakeup_ready) at the caller, what do you think? > @@ -695,12 +712,36 @@ int Repl_semi_sync_master::report_reply_binlog(uint32 server_id, > + lock(); > + while (!wait_queue.empty()) > + { > + semisync_wait_trx_t next_waiter= wait_queue.front(); > > - cond_broadcast(); > + cmp= Active_tranx::compare(m_reply_file_name, m_reply_file_pos, > + next_waiter.binlog_name, next_waiter.binlog_pos); > + if (cmp >= 0) > + { > + DBUG_PRINT("semisync", ("%s: signal thread %llu.", > + "Repl_semi_sync_master::report_reply_binlog", > + next_waiter.thd->thread_id)); > + mysql_cond_signal(&next_waiter.thd->COND_wakeup_ready); > + wait_queue.pop(); This code is very similar to Active_tranx::clear_active_tranx_nodes(), so hopefully the two can be integrated and wait_queue becomes unnecessary. > @@ -779,7 +821,20 @@ int Repl_semi_sync_master::report_binlog_update(THD* thd, const char *log_file, > strcpy(log_info->log_file, log_file + dirname_length(log_file)); > log_info->log_pos = log_pos; > > - return write_tranx_in_binlog(log_info->log_file, log_pos); > + /* > + THD arg depends on wait point mode. If after storage engine commit, the > + individual connection threads will perform the wait for semi-sync ACKt, > + thd is the thread of the user connection thread. > + If it is after binlog sync, the binlog leader thread will perform the > + semi-sync waits on behalf of the grouped transaction (which at this > + point, we (current_thd) are the leader). If using binlog_group_commit, > + thd is the thread of the user connection thread. > + */ > + thd_to_cond_wait= ((wait_point() == SEMI_SYNC_MASTER_WAIT_POINT_AFTER_STORAGE_COMMIT) > + ? thd > + : current_thd); This condition is complex. It's good that you added the detailed comment explaining what's going on, but I think it would be better to avoid it completely. Why not just pass in the value of thd_to_cond_wait explicitly from the caller? As an aside, we have this code in trx_group_commit_leader(): for (current= queue; current != NULL; current= current->next) { current->error= repl_semisync_master.wait_after_sync(current->cache_mngr-> last_commit_pos_file, current->cache_mngr-> last_commit_pos_offset); It seems silly to wait for every binlog position to be acked her one by one, why not just wait for the last one? repl_semisync_master.wait_after_sync(last_in_queue->cache_mngr->last_commit_pos_file, ...); But while related, it's perhaps not directly part of the problem you're addressing in your patch, so up to you if you want to include this or leave it for a possible later patch. > @@ -852,12 +907,11 @@ int Repl_semi_sync_master::commit_trx(const char* trx_wait_binlog_name, > - if (!get_master_enabled() || !is_on()) > + if (!get_master_enabled() || !is_on() || thd_killed(thd)) > goto l_end; > - while (is_on() && !thd_killed(thd)) > - { > - /* We have to check these again as things may have changed */ > - if (!rpl_semi_sync_master_clients && !rpl_semi_sync_master_wait_no_slave) > - { > - aborted= 1; > - break; > - } As described earlier, because of the (at least theoretical) possibility of spurious wakeup, we should keep the while loop here and the checks. > @@ -1253,6 +1294,8 @@ int Repl_semi_sync_master::write_tranx_in_binlog(const char* log_file_name, > } > else > { > + wait_queue.push({log_file_name, log_file_pos, thd}); Hopefully this can be instead folded into m_active_tranxs->insert_tranx_node() which is called just a bit below. > -void Repl_semi_sync_master::await_slave_reply() > +void Repl_semi_sync_master::await_all_slave_replies() > + while (TRUE) > + { > + lock(); > + if (wait_queue.empty() || !get_master_enabled() || !is_on()) > + { > + unlock(); > + break; > + } > + front= wait_queue.front(); > create_timeout(&abstime, NULL); > - cond_timewait(&abstime); > - > -end: > + wait_result= cond_timewait(front.thd, &abstime); I'm wondering, is it safe here to wait on front.thd->COND_wakeup_ready? Isn't there a possibility that this might go away during the wait? A simple way to avoid this could be just to keep the original global COND_binlog_send and wait for that here (and signal both that and thd->COND_wakeup_ready when waking up). IIUC, this code will only run in a single thread during shutdown, so probably fine to have some extra unnecessary wakeups here. > diff --git a/sql/semisync_master.h b/sql/semisync_master.h > index 99f46869354..6de0333d4ce 100644 > --- a/sql/semisync_master.h > +++ b/sql/semisync_master.h > @@ -21,6 +21,7 @@ > +/* > + Element in Repl_semi_sync_master::wait_queue to preserve the state of a > + transaction waiting for an ACK. > +*/ > +typedef struct _semisync_wait_trx { > + const char *binlog_name; > + my_off_t binlog_pos; > + THD *thd; > +} semisync_wait_trx_t; > + std::queue<semisync_wait_trx_t> wait_queue; Let's see if we can avoid this, and use the existing m_active_tranxs queue instead. diff --git a/sql/semisync_master.cc b/sql/semisync_master.cc index 8cc721e5737..0eaf0f0e0e2 100644 --- a/sql/semisync_master.cc +++ b/sql/semisync_master.cc @@ -979,6 +979,14 @@ int Repl_semi_sync_master::commit_trx(const char* trx_wait_binlog_name, { rpl_semi_sync_master_trx_wait_num++; rpl_semi_sync_master_trx_wait_time += wait_time; + /* + Assert we have either recieved our ACK; or have timed out and are + awoken in an off state. + */ + DBUG_ASSERT(!get_master_enabled() || !is_on() || thd->is_killed() || + 0 <= Active_tranx::compare( + m_reply_file_name, m_reply_file_pos, + trx_wait_binlog_name, trx_wait_binlog_pos)); } } } I think it's a bit much to crash the server here (even if only debug build), because of the possibility of spurious wakeups. It's good to have the test case though, maybe put the assertion inside DBUG_EXECUTE_IF()? I'd make it just log an error in the error log which the test case could flag (to avoid crashing), but that's a matter of style I guess and up to you. - Kristian.

2 2

Re: 95f2a07d323: MDEV-33582 Add more warnings to be able to better diagnose network issues
by Sergei Golubchik 05 Mar '24

05 Mar '24

Hi, Michael, On Mar 04, Michael Widenius wrote: > revision-id: 95f2a07d323 (mariadb-10.6.17-30-g95f2a07d323) > parent(s): 33dcf8155b4 > author: Michael Widenius > committer: Michael Widenius > timestamp: 2024-03-04 14:52:03 +0200 > message: > > MDEV-33582 Add more warnings to be able to better diagnose network issues > > Warnings are added to net_server.cc when > global_system_variables.log_warnings >= 4. > > When the above condition holds then: > - All communication errors from net_serv.cc is also written to the > general log. "to the error log" > - In case of a of not being able to read or write a packet, a more detailed "of a of" ? > error is given. > > Other things: > - vio_close() is now first marking the socket closed before closing it. > The reason for this is to ensure that the connection that gets a read > error can check if the reason was that the socket was closed. > - Added option to NET to avoid calling my_error() when there is no data > to be read on socket. This is only used in semi sync master to speed > up handling zero size packages that seams to happen from time to time. "seems" I don't understand why it happens and whether it's normal or not, and if normal, why it's normal only for semisync and not normal otherwise. > - Add a new state to vio to be able to detect if vio is acive, shutdown or > closed. > - Testing of the new warnings is done in rpl_get_lock.test > - Suppress some of the new warnings in mtr to allow one to run some of > the tests with -mysqld=--log-warnings=4. All test in the 'rpl' suite > can now be run with this option. > - Ensure that global.log_warnings are restored at test end in a way > that allows one to use mtr --mysqld=--log-warnings=4. > > diff --git a/mysql-test/mariadb-test-run.pl b/mysql-test/mariadb-test-run.pl > index 4b3d323760d..eabfe1171cc 100755 > --- a/mysql-test/mariadb-test-run.pl > +++ b/mysql-test/mariadb-test-run.pl > @@ -4486,6 +4486,11 @@ sub extract_warning_lines ($$) { > qr/Slave I\/0: Master command COM_BINLOG_DUMP failed/, > qr/Error reading packet/, > qr/Lost connection to MariaDB server at 'reading initial communication packet'/, > + qr/Could not read packet:.* state: [2-3] /, > + qr/Could not read packet:.* errno: 104 /, > + qr/Could not write packet:.* errno: 32 /, > + qr/Could not write packet:.* errno: 104 /, > + qr/Connection was killed/, why are these errors "normal" and can be ignored? > qr/Failed on request_dump/, > qr/Slave: Can't drop database.* database doesn't exist/, > qr/Slave: Operation DROP USER failed for 'create_rout_db'/, > diff --git a/mysql-test/suite/rpl/include/rpl_extra_col_master.test b/mysql-test/suite/rpl/include/rpl_extra_col_master.test > index 02645a577f5..512118f91cd 100644 > --- a/mysql-test/suite/rpl/include/rpl_extra_col_master.test > +++ b/mysql-test/suite/rpl/include/rpl_extra_col_master.test > @@ -59,6 +59,10 @@ > #VARCHAR(M) > # > > +--disable_query_log > +call mtr.add_suppression("Could not read packet:.* errno: 11"); > +--enable_query_log error 11? "Resource temporarily unavailable"? What does it mean in this context? > + > --let $_saved_conn= $CURRENT_CONNECTION > > let $binformat = `SHOW VARIABLES LIKE '%binlog_format%'`; > diff --git a/sql/semisync_master_ack_receiver.cc b/sql/semisync_master_ack_receiver.cc > index 9320c7fc3e6..1bb462ef934 100644 > --- a/sql/semisync_master_ack_receiver.cc > +++ b/sql/semisync_master_ack_receiver.cc > @@ -256,6 +256,12 @@ void Ack_receiver::run() > thd->security_ctx->skip_grants(); > thd->set_command(COM_DAEMON); > init_net(&net, net_buff, REPLY_MESSAGE_MAX_LENGTH); > + /* > + Don't generate error message in case of no data on socket. Will speed > + up my_net_read() slightly in case of NET_READ_INTERRUPTED. > + */ > + net.no_errors_for_no_data= 1; do you mean, it's for performance reasons? > + Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

Re: ea9a05f0821: This commit is about improving repair of tables
by Sergei Golubchik 04 Mar '24

04 Mar '24

Hi, Michael, First, instead of situationally fixing repair code here and there, let's take a step back and see how repair works. I've walked the whole REPAIR code line by line, including all storage engines, and that's a simplified description: * if USE_FRM was specified * < FRM_VER_TRUE_VARCHAR(≈2005) && has varchar -> "too old" * if not MyISAM or Aria or CSV -> silently ignore USE_FRM * rename/truncate/rename * if ALGORITHM=COPY is needed (and not FORCE/USE_FRM), use it * otherwise, per engine: * MyISAM run repair, QUICK or EXTENDED, or QUICK then EXTENDED * Aria, same as MyISAM * S3 -> "Table '%-.192s' is read only" * CSV crashes * Archive, perform OPTIMIZE * all other engines * if needs upgrade: use ALTER ALGORITHM=COPY * otherwise "The storage engine for the table doesn't support REPAIR" * if ALGORITHM=COPY is needed but wasn't used -> "Operation failed" I think it's too complex to explain and not user friendly. Note that MyISAM internally can try one repair method and if that fails fall back to another one. So does Aria. Why cannot the server do the same? So, I'd suggest the following logic instead: * if USE_FRM - do USE_FRM stuff (or an warning) * if marked corrupted or EXTENDED or doesn't need upgrade - run repair * if repair isn't supported = ALTER IGNORE unless needs upgrade * if needs upgrade - run upgrade = ALTER IGNORE COPY * if fails because of corruption - go to step 2 if repair wasn't run yet here USE_FRM is handled as before (except that it adds a warning if USE_FRM isn't supported by the engine). After that the server runs engine's repair if the table is marked corrupted or if EXTENDED was used (so no need for a new FORCE syntax). There is no error "engine does not support REPAIR", the server falls back to ALTER automatically in that case. If the data need to be upgraded and ALTER fails, the server tries repair and then ALTER again - just like MyISAM and Aria try different repair methods. Also, note that ALTER should always use IGNORE to delete duplicates in unique after collation of hash changes - that's what repair in MyISAM/Aria does. Anyway, some detailed comments to your patch are inline, please, see below. On Feb 21, Michael Widenius wrote: > commit ea9a05f0821 > Author: Michael Widenius <monty(a)mariadb.org> > Date: Mon Jan 29 11:52:44 2024 +0200 > > This commit is about improving repair of tables now as there is an MDEV, it should be here in the subject line > > The 3 ways to repair a table are: > 1) ALTER TABLE table_name FORCE" (not other options). > As an alias we allow: "ALTER TABLE table_name ENGINE=original_engine" > 2) "REPAIR TABLE" (without FORCE) > 3) "OPTIMIZE TABLE" > > All of the above commands will optimize row space usage (which means that > space will be needed to hold a temporary copy of the table) and > re-genereate all indexes. They will also try to replicate the original > table definition as exact as possible. > > For ALTER TABLE and "REPAIR TABLE without FORCE", the following will hold: > If the table is from an older MariaDB version and data conversion is > needed (for example for old type HASH columns, MySQL JSON type or new > TIMESTAMP format) "ALTER TABLE table_name FORCE, algorithm=COPY" will be > used. > > The differences between the algoritms are > 1) Will use the fastest algorithm the engine supports to do a full repair > of the table (except if data conversions are is needed). > 2) Will use the storage engine internal REPAIR facility (MyISAM, Aria). > If the engine does not support REPAIR then > "ALTER TABLE FORCE, ALGORITHM=COPY" will be used. > If there was data incompatibilites (which means that FORCE was used) > then there will be a warning after REPAIR that ALTER TABLE FORCE is > still needed. > The reason for this is that REPAIR may be able to go around data > errors (wrong incompatible data, crashed or unreadable sectors) that > ALTER TABLE cannot do. > 3) Will use the storage engine internal OPTIMIZE. If engine does not > support optimize, then "ALTER TABLE FORCE" is used. > > The above will ensure that ALTER TABLE FORCE is able to > correct almost any errors in the row or index data. In case of > corrupted blocks then REPAIR possibile followed by ALTER TABLE is needed. > This is important as mariadb-upgrade executes ALTER TABLE table_name > FORCE for any table that must be re-created. > > Bugs fixed with InnoDB tables: > - No error for INNODB_DEFAULT_ROW_FORMAT=COMPACT even if row length > would be too wide. (Independent of innodb_strict_mode). > - Tables using symlinks will be symlinked after any of the above commands > (Independent of the setting of --symbolic-links) > > If one specifies an algorithm together with ALTER TABLE FORCE, things > will work as before (except if data conversion is required as then > the COPY algoritm is enforced). > > ALTER TABLE .. OPTIMIZE ALL PARTITIONS will work as before. > > Other things: > - FORCE argument added to REPAIR to allow one to first run internal > repair to fix damaged blocks and then follow it with ALTER TABLE. > - REPAIR will not update frm_version if ha_check_for_upgrade() finds > that table is still incompatible with current version. In this case the > REPAIR will end with an error. doesn't look like "the REPAIR will end with an error" > - Default error messages length for %M increased from 128 to 256 to not > cut information from REPAIR. > - Documented HA_ADMIN_XX variables related to repair. > - Added HA_ADMIN_NEEDS_DATA_CONVERSION to signal that we have to > do data conversions when converting the table (and thus ALTER TABLE > copy algorithm is needed). > > diff --git a/include/my_handler_errors.h b/include/my_handler_errors.h > index df414888907..06dde5b3f74 100644 > --- a/include/my_handler_errors.h > +++ b/include/my_handler_errors.h > @@ -101,7 +101,7 @@ static const char *handler_error_messages[]= > "Operation was interrupted by end user (probably kill command?)", > "Disk full", > /* 190 */ > - "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You may have retry or dump and restore the table to fix this", > + "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this", 1. typo. "possibly" 2. please, don't recommend USE_FRM, it's a last resort lossy method, designed only for the case when MYI file is lost or corrupted. 3. REPAIR USE_FRM is likey broken for CSV, but I couldn't test it, because REPAIR crashes on CSV tables with or without USE_FRM. > "Too many words in a FTS phrase or proximity search", > "Table encrypted but decryption failed. This could be because correct encryption management plugin is not loaded, used encryption key is not available or encryption method does not match.", > "Foreign key cascade delete/update exceeds max depth", > diff --git a/mysql-test/main/ctype_utf8_def_upgrade.result b/mysql-test/main/ctype_utf8_def_upgrade.result > index d30f8670536..e7c21b76058 100644 > --- a/mysql-test/main/ctype_utf8_def_upgrade.result > +++ b/mysql-test/main/ctype_utf8_def_upgrade.result > @@ -10,24 +10,39 @@ SELECT @@character_set_database; > latin1 > SET @@character_set_database="latin1"; > SELECT COUNT(*) FROM t1; > -ERROR HY000: Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You may have retry " from storage engine MyISAM > +ERROR HY000: Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this" from storage engine MyISAM > +ALTER TABLE t1 FORCE; > +ERROR HY000: Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this" from storage engine MyISAM > CHECK TABLE t1; > Table Op Msg_type Msg_text > -test.t1 check Error Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You may have retry " from storage engine MyISAM > +test.t1 check Error Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this" from storage engine MyISAM > test.t1 check error Corrupt > REPAIR TABLE t1; > Table Op Msg_type Msg_text > -test.t1 repair Error Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You may have retry " from storage engine MyISAM > +test.t1 repair Error Got error 190 "Incompatible key or row definition between the MariaDB .frm file and the information in the storage engine. You can try REPAIR TABLE ... USE_FRM possible followed by ALTER TABLE ... FORCE or dump and restore the table to fix this" from storage engine MyISAM > +test.t1 repair error Corrupt > +REPAIR TABLE t1 FORCE; > +Table Op Msg_type Msg_text > +test.t1 repair Warning Can't open table > test.t1 repair error Corrupt > REPAIR TABLE t1 USE_FRM; > Table Op Msg_type Msg_text > -test.t1 repair status OK > +test.t1 repair note Table data recovered > +t1 repair error Table rebuild required. Please do "ALTER TABLE `test.t1` FORCE" or dump/reload to fix it! better to avoid exclamation marks in the error messages! > +test.t1 repair status Operation failed > SELECT COUNT(*) FROM t1; > COUNT(*) > 0 > CHECK TABLE t1; > Table Op Msg_type Msg_text > +test.t1 check error Table rebuild required. Please do "ALTER TABLE `t1` FORCE" or dump/reload to fix it! > +ALTER TABLE t1 FORCE; > +CHECK TABLE t1; > +Table Op Msg_type Msg_text > test.t1 check status OK > +SELECT COUNT(*) FROM t1; > +COUNT(*) > +0 > SHOW CREATE TABLE t1; > Table Create Table > t1 CREATE TABLE `t1` ( > diff --git a/sql/handler.h b/sql/handler.h > index fe272d841d9..f429cdacc6f 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -2249,6 +2267,8 @@ struct Table_scope_and_contents_source_pod_st // For trivial members > enum_stats_auto_recalc stats_auto_recalc; > bool varchar; ///< 1 if table has a VARCHAR > bool sequence; // If SEQUENCE=1 was used > + /* True if we are using OPTIMIZE TABLE, REPAIR TABLE or ALTER TABLE FORCE */ it'd be good to clarify here /* this is used to suppress certain warnings that are only used for CREATE/ALTER TABLE */ > + bool recreate_identical_table; > > List<Virtual_column_info> *check_constraint_list; > > diff --git a/sql/sql_admin.cc b/sql/sql_admin.cc > index fcbd8a55c15..d4eabd7c34a 100644 > --- a/sql/sql_admin.cc > +++ b/sql/sql_admin.cc > @@ -48,7 +48,8 @@ const LEX_CSTRING msg_optimize= { STRING_WITH_LEN("optimize") }; > /* Prepare, run and cleanup for mysql_recreate_table() */ > > static bool admin_recreate_table(THD *thd, TABLE_LIST *table_list, > - Recreate_info *recreate_info) > + Recreate_info *recreate_info, > + bool partition_admin, bool table_copy) you don't need to pass partition_admin as an argument, if it's always thd->lex->alter_info.partition_flags & ALTER_PARTITION_ADMIN > { > bool result_code; > DBUG_ENTER("admin_recreate_table"); > @@ -859,20 +867,29 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, > /* purecov: end */ > } > > - if (operator_func == &handler::ha_repair && > - !(check_opt->sql_flags & TT_USEFRM)) > + if (operator_func == &handler::ha_repair) > { > handler *file= table->table->file; > int check_old_types= file->check_old_types(); > int check_for_upgrade= file->ha_check_for_upgrade(check_opt); why check_old_types is not part of ha_check_for_upgrade? they're always used together, as far as I can see. > + require_data_conversion= > + (check_old_types == HA_ADMIN_NEEDS_DATA_CONVERSION || > + check_for_upgrade == HA_ADMIN_NEEDS_DATA_CONVERSION); > + require_alter_table= > + (check_old_types == HA_ADMIN_NEEDS_ALTER || > + check_for_upgrade == HA_ADMIN_NEEDS_ALTER); > > - if (check_old_types == HA_ADMIN_NEEDS_ALTER || > - check_for_upgrade == HA_ADMIN_NEEDS_ALTER) > + if (!(check_opt->sql_flags & (TT_USEFRM | TT_FORCE))) > + { > + if (require_data_conversion || require_alter_table) > { > /* We use extra_open_options to be able to open crashed tables */ > thd->open_options|= extra_open_options; > - result_code= admin_recreate_table(thd, table, &recreate_info) ? > + result_code= admin_recreate_table(thd, table, &recreate_info, > + (lex->alter_info.partition_flags & > + ALTER_PARTITION_ADMIN), 1) ? > HA_ADMIN_FAILED : HA_ADMIN_OK; > + recreate_used= 1; > thd->open_options&= ~extra_open_options; > goto send_result; > } > if (check_old_types || check_for_upgrade) > { > /* If repair is not implemented for the engine, run ALTER TABLE */ > need_repair_or_alter= 1; > } > } > - > + } > result_code= compl_result_code= HA_ADMIN_OK; > > if (operator_func == &handler::ha_analyze) > @@ -1229,7 +1277,10 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, > *save_next_global= table->next_global; > table->next_local= table->next_global= 0; > > - result_code= admin_recreate_table(thd, table, &recreate_info); > + result_code= admin_recreate_table(thd, table, &recreate_info, > + (lex->alter_info.partition_flags & > + ALTER_PARTITION_ADMIN), 0); > + recreate_used= 1; that's a bit too late, you check recreate_used earlier > trans_commit_stmt(thd); > trans_commit(thd); > close_thread_tables(thd); > @@ -1641,7 +1696,8 @@ bool Sql_cmd_repair_table::execute(THD *thd) > WSREP_TO_ISOLATION_BEGIN_WRTCHK(NULL, NULL, first_table); > res= mysql_admin_table(thd, first_table, &m_lex->check_opt, &msg_repair, > TL_WRITE, 1, > - MY_TEST(m_lex->check_opt.sql_flags & TT_USEFRM), > + MY_TEST(m_lex->check_opt.sql_flags & > + (TT_USEFRM | TT_FORCE)), better rename repair_table_use_frm argument of mysql_admin_table and open_only_one_table, because it's not only TT_USEFRM anymore. a good name could be no_errors_from_open > HA_OPEN_FOR_REPAIR, &prepare_for_repair, > &handler::ha_repair, &view_repair, true); > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 55319a2242f..7d5a968070a 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -10868,19 +10869,56 @@ do_continue:; > } > #endif /* WITH_WSREP */ > > + /* > + ALTER TABLE ... ENGINE to the same engine is a common way to > + request table rebuild. Set ALTER_RECREATE flag to force table > + rebuild. > + */ > + if (new_db_type == old_db_type) > + { > + if (create_info->used_fields & HA_CREATE_USED_ENGINE) > + alter_info->flags|= ALTER_RECREATE; > + > + /* > + Check if we are using ALTER TABLE FORCE without any other options > + (except ENGINE == current_engine). > + In this case we will try to recreate an identical to the original. > + This code is also used with REPAIR and OPTIMIZE. > + */ > + if (alter_info->flags == ALTER_RECREATE && > + ((create_info->used_fields & ~HA_CREATE_USED_ENGINE) == 0)) > + create_info->recreate_identical_table= 1; > + } > + > /* > We can use only copy algorithm if one of the following is true: > + - If the table is from an old MariaDB version and requires data > + modifications. In this case we ignore --alter-algorithm as eh, no, you cannot ignore it, if the user specified incompatible set of options, ALTER TABLE has to fail with an error, saying ALGORITHM=INPLACE (for example) is impossible here. The server cannot just silently ignore an explicit user command. > + as we cannot use any other algorithm than COPY (using other > + algorithms could open up the problem that the table .frm version > + is updated without data transformations and the table would be > + corrupted without any way for MariaDB to notice this during > + check/upgrade). > + This logic ensurses that ALTER TABLE ... FORCE (no other > + options) will always be be able to repair a table structure and > + convert data from any old format. > - In-place is impossible for given operation. > - Changes to partitioning which were not handled by fast_alter_part_table() > needs to be handled using table copying algorithm unless the engine > supports auto-partitioning as such engines can do some changes > using in-place API. > */ > + check_opt.init(); > + > + require_copy_algorithm= (table->file->ha_check_for_upgrade(&check_opt) == > + HA_ADMIN_NEEDS_DATA_CONVERSION); > + if (require_copy_algorithm || > - if (is_inplace_alter_impossible(table, create_info, alter_info) > - || IF_PARTITIONING((partition_changed && > + is_inplace_alter_impossible(table, create_info, alter_info) || > + IF_PARTITIONING((partition_changed && > !(old_db_type->partition_flags() & HA_USE_AUTO_PARTITION)), 0)) > { > - if (alter_info->algorithm_is_nocopy(thd)) > + if (alter_info->algorithm_is_nocopy(thd) && > + !(require_copy_algorithm && alter_info->algorithm_not_specified())) > { > my_error(ER_ALTER_OPERATION_NOT_SUPPORTED, MYF(0), > alter_info->algorithm_clause(thd), "ALGORITHM=COPY"); > @@ -12451,10 +12482,14 @@ bool mysql_recreate_table(THD *thd, TABLE_LIST *table_list, > create_info.init(); > create_info.row_type=ROW_TYPE_NOT_USED; > create_info.alter_info= &alter_info; > + create_info.recreate_identical_table= 1; > /* Force alter table to recreate table */ > alter_info.flags= (ALTER_CHANGE_COLUMN | ALTER_RECREATE); > + alter_info.partition_flags= thd->lex->alter_info.partition_flags; > + if (partition_admin) > + alter_info.partition_flags= ALTER_PARTITION_ADMIN; when is this needed? I couldn't find a single code path where alter_info.partition_flags is not ALTER_PARTITION_ADMIN at this point. Neither code analysys nor mtr have found anything. You can safely remove this if() and add DBUG_ASSERT(alter_info.partition_flags & ALTER_PARTITION_ADMIN); > > - if (table_copy) > + if (table_copy && !partition_admin) > alter_info.set_requested_algorithm( > Alter_info::ALTER_TABLE_ALGORITHM_COPY); > > diff --git a/sql/table.cc b/sql/table.cc > index f84eeda297f..b09e9b11f17 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -1846,6 +1846,8 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > */ > if (share->frm_version == FRM_VER_TRUE_VARCHAR -1 && frm_image[33] == 5) > share->frm_version= FRM_VER_TRUE_VARCHAR; > + if (share->frm_version <= FRM_VER_TRUE_VARCHAR) > + share->keep_original_mysql_version= 1; this is wrong. see prepare_frm_header(), frm_version is *always* FRM_VER_TRUE_VARCHAR if there are no vcols. you've basically just disabled update_frm_version() for almost everyone. > > new_field_pack_flag= frm_image[27]; > new_frm_ver= (frm_image[2] - FRM_VER); Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 3

RISC-V CI for MariaDB
by ali.tariq＠10xengineers.ai 04 Mar '24

04 Mar '24

Hi all, I have seen the status of MariaDB in the RISC-V software ecosystem sheet (https://docs.google.com/spreadsheets/d/10rWj7QlzCiXwTZ778vex5_nRLP9gLrNC8Pw…) as "SUPPORTED" but not yet "FULLY SUPPORTED" or "Optimized" for RISC-V architecture, which means optimization for RISC-V is still in progress. I want to bump a CI platform called Cloud-V (https://cloud-v.co/) (created in collaboration with the RISC-V Labs subgroup) which provides CI for RISC-V with emulated and physical RISC-V computes. You can enroll for access at this link (https://cloud-v.co/contactus)

1 0

Storing binlig in InnoDB/engine
by Kristian Nielsen 28 Feb '24

28 Feb '24

Hi Marko, I looked a bit more at the idea that we have discussed a couple times, of storing the binlog in an InnoDB tablespace to avoid the need for two-phase commit between binlog and InnoDB and save one (or potentially both) fsync()s during a commit. I managed to get some InnoDB code written that is able to create a tablespace and write to it, patch below or on github: https://github.com/MariaDB/server/commits/knielsen_binlog_in_engine https://github.com/MariaDB/server/commit/95958c3842ebf0f7e358d6c3f51b887bd9… This is far from a complete patch, just an exercise for me on how InnoDB tablespaces and mini-transactions work in detail. But it turned out to be useful for me to get started and find a few questions to ask. My general approach to writing is to use fsp_page_create() for the first write to a page, and then buf_page_get_gen() for subsequent writes. But maybe this should be refined for the actual implementation. I'm thinking if perhaps fsp_page_create() does too much, you mentioned earlier that some parts of the page header could be simplified/omitted. And maybe there is a way to pin the current page in the buffer pool so buf_page_get_gen() is not needed for every write? I'm currently passing RW_SX_LATCH to buf_page_get_gen() (otherwise I got an assertion when writing). I'm not sure though how these latches work, or if binlog writing would need such latches; maybe it makes more sense to have a simple mutex protecting page access? Another assertion was fixed by doing mtr.set_named_space() before writing. Again, I'm not sure what this does exactly or if it's appropriate? I tried in this patch to reserve 2 "special" tablespace ids for the binlog tablespaces. Idea would be to cycle between them, keeping at most the two last tablespaces active. But do the tablespace IDs appear in the redo log and used for recovery? In that case, I assume that all binlog tablespaces written since the last InnoDB checkpoint will need a unique tablespace ID? So maybe 2 is too few. I was thinking maybe the binlog could allocate new tablespace IDs as necessary, but re-use them after each InnoDB checkpoint. This would avoid wasting ids and eventually hitting the 2**32 limit. - Kristian. commit 95958c3842ebf0f7e358d6c3f51b887bd9948845 (HEAD -> binlog_in_inno, origin/knielsen_binlog_in_engine) Author: Kristian Nielsen <knielsen(a)knielsen-hq.org> Date: Sun Feb 25 17:41:50 2024 +0100 Binlog in Engine: Very first sketch, able to create and write an InnoDB tablespace Signed-off-by: Kristian Nielsen <knielsen(a)knielsen-hq.org> diff --git a/mysql-test/suite/binlog/t/binlog_in_engine.test b/mysql-test/suite/binlog/t/binlog_in_engine.test new file mode 100644 index 00000000000..947139c9bcc --- /dev/null +++ b/mysql-test/suite/binlog/t/binlog_in_engine.test @@ -0,0 +1,11 @@ +--source include/have_innodb.inc +--source include/have_binlog_format_mixed.inc + +CREATE TABLE t1 (a INT PRIMARY KEY) ENGINE=InnoDB; +INSERT INTO t1 VALUES (1); +BEGIN; +INSERT INTO t1 VALUES (2); +INSERT INTO t1 VALUES (3); +COMMIT; +SELECT * FROM t1 ORDER BY a; +DROP TABLE t1; diff --git a/storage/innobase/buf/buf0flu.cc b/storage/innobase/buf/buf0flu.cc index bd43429eb5d..09d8fdaff39 100644 --- a/storage/innobase/buf/buf0flu.cc +++ b/storage/innobase/buf/buf0flu.cc @@ -1626,7 +1626,8 @@ static ulint buf_flush_list(ulint max_n= ULINT_UNDEFINED, bool buf_flush_list_space(fil_space_t *space, ulint *n_flushed) { const auto space_id= space->id; - ut_ad(space_id <= SRV_SPACE_ID_UPPER_BOUND); + ut_ad(space_id <= SRV_SPACE_ID_UPPER_BOUND || + space_id == SRV_SPACE_ID_BINLOG0 || space_id == SRV_SPACE_ID_BINLOG1); bool may_have_skipped= false; ulint max_n_flush= srv_io_capacity; diff --git a/storage/innobase/fil/fil0fil.cc b/storage/innobase/fil/fil0fil.cc index 0ce54df6574..e83225a4883 100644 --- a/storage/innobase/fil/fil0fil.cc +++ b/storage/innobase/fil/fil0fil.cc @@ -184,7 +184,7 @@ it is an absolute path. */ const char* fil_path_to_mysql_datadir; /** Common InnoDB file extensions */ -const char* dot_ext[] = { "", ".ibd", ".isl", ".cfg" }; +const char* dot_ext[] = { "", ".ibd", ".isl", ".cfg", ".ibb" }; /** Number of pending tablespace flushes */ Atomic_counter<ulint> fil_n_pending_tablespace_flushes; @@ -1044,6 +1044,9 @@ fil_space_t *fil_space_t::create(uint32_t id, uint32_t flags, if (UNIV_LIKELY(id <= fil_system.max_assigned_id)) { break; } + if (id == SRV_SPACE_ID_BINLOG0 || id == SRV_SPACE_ID_BINLOG1) { + break; + } if (UNIV_UNLIKELY(srv_operation == SRV_OPERATION_BACKUP)) { break; } @@ -1603,9 +1606,10 @@ inline void mtr_t::log_file_op(mfile_type_t type, uint32_t space_id, ut_ad(!(byte(type) & 15)); /* fil_name_parse() requires that there be at least one path - separator and that the file path end with ".ibd". */ + separator and that the file path end with ".ibd" or "ibb". */ ut_ad(strchr(path, '/')); - ut_ad(!strcmp(&path[strlen(path) - strlen(DOT_IBD)], DOT_IBD)); + ut_ad(!strcmp(&path[strlen(path) - strlen(DOT_IBD)], DOT_IBD) || + !strcmp(&path[strlen(path) - strlen(DOT_IBB)], DOT_IBB)); m_modifications= true; if (!is_logged()) diff --git a/storage/innobase/fsp/fsp0fsp.cc b/storage/innobase/fsp/fsp0fsp.cc index 787bda53895..f1bb42b3a15 100644 --- a/storage/innobase/fsp/fsp0fsp.cc +++ b/storage/innobase/fsp/fsp0fsp.cc @@ -3763,3 +3763,129 @@ void fsp_shrink_temp_space() mtr.commit(); sql_print_information("InnoDB: Temporary tablespace truncated successfully"); } + + + +fil_space_t* binlog_space; +buf_block_t *binlog_cur_block; +uint32_t binlog_cur_page_no; +uint32_t binlog_cur_page_offset; + +/** Create a binlog tablespace file +@param[in] name file name +@return DB_SUCCESS or error code */ +dberr_t fsp_binlog_tablespace_create(const char* name) +{ + pfs_os_file_t fh; + bool ret; + + uint32_t size= (1<<20) >> srv_page_size_shift /* ToDo --max-binlog-size */; + if(srv_read_only_mode) + return DB_ERROR; + + os_file_create_subdirs_if_needed(name); + + /* ToDo: Do we need here an mtr.log_file_op(FILE_CREATE) like in fil_ibd_create(()? */ + fh = os_file_create( + innodb_data_file_key, + name, + OS_FILE_CREATE | OS_FILE_ON_ERROR_NO_EXIT, + OS_FILE_AIO, OS_DATA_FILE, srv_read_only_mode, &ret); + + if (!ret) { + os_file_close(fh); + return DB_ERROR; + } + + /* ToDo: Enryption? */ + fil_encryption_t mode= FIL_ENCRYPTION_OFF; + fil_space_crypt_t* crypt_data= nullptr; + + /* We created the binlog file and now write it full of zeros */ + if (!os_file_set_size(name, fh, + os_offset_t{size} << srv_page_size_shift)) { + ib::error() << "Unable to allocate " << name; + os_file_close(fh); + os_file_delete(innodb_data_file_key, name); + return DB_ERROR; + } + + mysql_mutex_lock(&fil_system.mutex); + uint32_t space_id= SRV_SPACE_ID_BINLOG0; + if (!(binlog_space= fil_space_t::create(space_id, + ( FSP_FLAGS_FCRC32_MASK_MARKER | + FSP_FLAGS_FCRC32_PAGE_SSIZE()), + FIL_TYPE_TABLESPACE, crypt_data, + mode, true))) { + mysql_mutex_unlock(&fil_system.mutex); + return DB_ERROR; + } + + fil_node_t* node = binlog_space->add(name, fh, size, false, true); + IF_WIN(node->find_metadata(), node->find_metadata(fh, true)); + mysql_mutex_unlock(&fil_system.mutex); + + binlog_cur_page_no= 0; + binlog_cur_page_offset= FIL_PAGE_DATA; + return DB_SUCCESS; +} + +void fsp_binlog_write_start(uint32_t page_no, + const uchar *data, uint32_t len, mtr_t *mtr) +{ + buf_block_t *block= fsp_page_create(binlog_space, page_no, mtr); + mtr->memcpy<mtr_t::MAYBE_NOP>(*block, FIL_PAGE_DATA + block->page.frame, + data, len); + binlog_cur_block= block; +} + +void fsp_binlog_write_offset(uint32_t page_no, uint32_t offset, + const uchar *data, uint32_t len, mtr_t *mtr) +{ + dberr_t err; + /* ToDo: Is RW_SX_LATCH appropriate here? */ + buf_block_t *block= buf_page_get_gen(page_id_t{binlog_space->id, page_no}, + 0, RW_SX_LATCH, binlog_cur_block, + BUF_GET, mtr, &err); + ut_a(err == DB_SUCCESS); + mtr->memcpy<mtr_t::MAYBE_NOP>(*block, + offset + block->page.frame, + data, len); +} + +void fsp_binlog_append(const uchar *data, uint32_t len, mtr_t *mtr) +{ + ut_ad(binlog_cur_page_offset <= srv_page_size - FIL_PAGE_DATA_END); + uint32_t remain= ((uint32_t)srv_page_size - FIL_PAGE_DATA_END) - + binlog_cur_page_offset; + // ToDo: Some kind of mutex to protect binlog access. + while (len > 0) { + if (remain < 4) { + binlog_cur_page_offset= FIL_PAGE_DATA; + remain= ((uint32_t)srv_page_size - FIL_PAGE_DATA_END) - + binlog_cur_page_offset; + ++binlog_cur_page_no; + } + uint32_t this_len= std::min<uint32_t>(len, remain); + if (binlog_cur_page_offset == FIL_PAGE_DATA) + fsp_binlog_write_start(binlog_cur_page_no, data, this_len, mtr); + else + fsp_binlog_write_offset(binlog_cur_page_no, binlog_cur_page_offset, + data, this_len, mtr); + len-= this_len; + data+= this_len; + binlog_cur_page_offset+= this_len; + } +} + + +void fsp_binlog_test(const uchar *data, uint32_t len) +{ + mtr_t mtr; + mtr.start(); + if (!binlog_space) + fsp_binlog_tablespace_create("./binlog-000000.ibb"); + mtr.set_named_space(binlog_space); + fsp_binlog_append(data, len, &mtr); + mtr.commit(); +} diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 93127bb1c3a..df2bd07d2dc 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -4481,6 +4481,10 @@ innobase_commit( if (commit_trx || (!thd_test_options(thd, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))) { + /* ToDo: This is just a random very initial test of writing + something into a binlog tablespace. */ + if (!opt_bootstrap) + fsp_binlog_test((const uchar *)"Hulubulu!!?!", 12); /* Run the fast part of commit if we did not already. */ if (!trx->active_commit_ordered) { innobase_commit_ordered_2(trx, thd); diff --git a/storage/innobase/include/fil0fil.h b/storage/innobase/include/fil0fil.h index f3660eff7c6..17b35f2f892 100644 --- a/storage/innobase/include/fil0fil.h +++ b/storage/innobase/include/fil0fil.h @@ -1129,10 +1129,12 @@ enum ib_extention { NO_EXT = 0, IBD = 1, ISL = 2, - CFG = 3 + CFG = 3, + IBB = 4 }; extern const char* dot_ext[]; #define DOT_IBD dot_ext[IBD] +#define DOT_IBB dot_ext[IBB] #define DOT_ISL dot_ext[ISL] #define DOT_CFG dot_ext[CFG] diff --git a/storage/innobase/include/fsp0fsp.h b/storage/innobase/include/fsp0fsp.h index ddc45e53fe6..26a45518ba2 100644 --- a/storage/innobase/include/fsp0fsp.h +++ b/storage/innobase/include/fsp0fsp.h @@ -579,6 +579,8 @@ void fsp_system_tablespace_truncate(); /** Truncate the temporary tablespace */ void fsp_shrink_temp_space(); +extern void fsp_binlog_test(const uchar *data, uint32_t len); + #ifndef UNIV_DEBUG # define fsp_init_file_page(space, block, mtr) fsp_init_file_page(block, mtr) #endif diff --git a/storage/innobase/include/fsp0types.h b/storage/innobase/include/fsp0types.h index 757ead55d03..e3d45796190 100644 --- a/storage/innobase/include/fsp0types.h +++ b/storage/innobase/include/fsp0types.h @@ -27,8 +27,12 @@ Created May 26, 2009 Vasil Dimov #pragma once #include "ut0byte.h" -/** All persistent tablespaces have a smaller fil_space_t::id than this. */ +/** All persistent tablespaces (except binlog tablespaces) have a smaller +fil_space_t::id than this. */ constexpr uint32_t SRV_SPACE_ID_UPPER_BOUND= 0xFFFFFFF0U; +/** Binlog tablespaces. */ +constexpr uint32_t SRV_SPACE_ID_BINLOG0 = SRV_SPACE_ID_UPPER_BOUND + 1; +constexpr uint32_t SRV_SPACE_ID_BINLOG1 = SRV_SPACE_ID_UPPER_BOUND + 2; /** The fil_space_t::id of the innodb_temporary tablespace. */ constexpr uint32_t SRV_TMP_SPACE_ID= 0xFFFFFFFEU; diff --git a/storage/innobase/include/mtr0log.h b/storage/innobase/include/mtr0log.h index e2419309764..86f6e3794f6 100644 --- a/storage/innobase/include/mtr0log.h +++ b/storage/innobase/include/mtr0log.h @@ -25,7 +25,7 @@ Mini-transaction log record encoding and decoding #include "mtr0mtr.h" /** The smallest invalid page identifier for persistent tablespaces */ -constexpr page_id_t end_page_id{SRV_SPACE_ID_UPPER_BOUND, 0}; +constexpr page_id_t end_page_id{SRV_SPACE_ID_BINLOG1, 0}; /** The minimum 2-byte integer (0b10xxxxxx xxxxxxxx) */ constexpr uint32_t MIN_2BYTE= 1 << 7; diff --git a/storage/innobase/log/log0recv.cc b/storage/innobase/log/log0recv.cc index ef31a4d00c1..310acb73071 100644 --- a/storage/innobase/log/log0recv.cc +++ b/storage/innobase/log/log0recv.cc @@ -2973,7 +2973,8 @@ recv_sys_t::parse_mtr_result recv_sys_t::parse(source &l, bool if_exists) if (is_predefined_tablespace(space_id)) goto file_rec_error; - if (fnend - fn < 4 || memcmp(fnend - 4, DOT_IBD, 4)) + if (fnend - fn < 4 || + (memcmp(fnend - 4, DOT_IBD, 4) && memcmp(fnend - 4, DOT_IBB, 4))) goto file_rec_error; if (UNIV_UNLIKELY(!recv_needed_recovery && srv_read_only_mode))

2 5

[PATCH 0/2] Suggestion for smaller fix to XA parallel replication performance, MDEV-31949
by Kristian Nielsen 28 Feb '24

28 Feb '24

As was discussed recently off-list, there is a desire to find a minimal fix to the performance issues in 10.6 with parallel replication of user XA due to non-optimal scheduling (MDEV-31949). How about something like this two-patch series? It _only_ modifies the scheduling of event groups to worker threads, and only for XA transactions, and is quite a bit simpler than the other proposed patches for MDEV-31949. The current 10.6 code for parallel replication of user XA needs to avoid applying in parallel two different event groups that have the same XA XID. Typically the XA PREPARE and corresponding XA COMMIT/ROLLBACK, but could also be another transaction that uses a duplicate XID. The method currently used in 10.6 takes a hash of the XID and assigns the transaction to worker (hash MOD N) amongst the N workers. This is sub-optimal, since there is a high chance that amonst N transactions, several of them will map to the same worker. Thus less than N transactions will be able to replicate in parallel and group commit together. The second patch in the series improves this by keeping track of recently active XIDs and which worker they were scheduled on. A subsequent event group with the same XID is scheduled on the same worker. If the XID is different, the event group is scheduled normally on the next free worker in round-robin fashion. This permits most XA PREPARE to be scheduled without dependency restrictions, same as normal non-XA transactions. Only if the user submits multiple XA transactions close together using the same duplicate XID will there be dependency restrictions. The XA COMMIT will normally be scheduled on the same worker as the XA PREPARE, unless the two events are far apart in the replication stream. This is mostly unavoidable in the current XA replication design, since the XA PREPARE and XA COMMIT of a single transaction cannot group-commit together. The first patch is a refactor/preparation patch. It rewrites the scheduling to use an explicit FIFO for scheduling the threads instead of a simple `i := (i+1) % N` cyclic counter. This allows to combine explicit scheduling of some transactions with round-robin scheduling of the rest in the second patch. Patches also available on github: https://github.com/MariaDB/server/commits/knielsen_xa_sched_minimal_fix https://github.com/MariaDB/server/commit/66d6cce96f831b638812490844d7542317… https://github.com/MariaDB/server/commit/3a32fb546c111e4627cad7c66bcc089bc1… This is just a quick proof-of-concept I did today, but it seems to work, and is small and simple and suitable for targeted fixing and cherry-picking. Brandon and/or Andrei, maybe you could try your benchmarks with this patch and see if it also solves the performance issues you were seeing? Or if there's something else required for XA that I'm missing? - Kristian. Kristian Nielsen (2): Refactor parallel replication round-robin scheduling to use explicit FIFO More precise dependency tracking of XA XID in parallel replication sql/rpl_parallel.cc | 158 ++++++++++++++++++++++++++++++++++++++------ sql/rpl_parallel.h | 48 +++++++++++++- 2 files changed, 181 insertions(+), 25 deletions(-) -- 2.30.2

1 3

Fwd: 2342aaf46eb: MDEV-30973 Implement debug logging for slave_transaction_retries
by Aleksey Midenkov 28 Feb '24

28 Feb '24

Hi Sergei, the commits are in bb-11.4-midenok-MDEV-30973 New changes are in 4 top commits. There is also a small CMake change "Columnstore empty submodule fix" which I would push ASAP. On Tue, Nov 7, 2023 at 8:35 PM Sergei Golubchik <serg(a)mariadb.org> wrote: > > Hi, Aleksey, > > On Nov 07, Aleksey Midenkov wrote: > > revision-id: 2342aaf46eb (mariadb-11.0.1-256-g2342aaf46eb) > > parent(s): 80bc5df1c80 > > author: Aleksey Midenkov > > committer: Aleksey Midenkov > > timestamp: 2023-10-10 13:11:19 +0300 > > message: > > > > MDEV-30973 Implement debug logging for slave_transaction_retries > > > > Configuration options: > > > > log_slave_retries > > log_slave_retries_max > > 1. > a commit comment of >100 lines that's a bit too much, I think. > > 2. > comments in MDEV-30973 say that one should be able to have everything in > one file, namely in the error log. I don't see how it's possible in your > commit. Fixed. > > 3. > one of the reasons for not simply writing into erorr log was > "you get what are relatively large entries in there which can make it > hard/annoying to also look for the normal, one-line warning and error > output". But as far as I can see your slave retry entries are not large, > but normal one-line messages. I'd say they can fit into the error log > just fine. "large entries" here is not fully correct. "large number of entries" is more correct. > > I've still commented below in the assumption that the separate log > implementation will stay. But I honestly don't see a reason for it > anymore. > > > diff --git a/mysql-test/suite/rpl/t/slave_deadlock.test b/mysql-test/suite/rpl/t/slave_deadlock.test > > new file mode 100644 > > index 00000000000..9fcda7c9b71 > > --- /dev/null > > +++ b/mysql-test/suite/rpl/t/slave_deadlock.test > > @@ -0,0 +1,194 @@ > > +--source include/have_debug.inc > > +--source include/have_innodb.inc > > +--source include/have_binlog_format_statement.inc > > + > > +# On --repeat test infinitely increments thread_id and finally fails --sync_with_master at log_pos 1754: > > +# > > +# 231009 20:58:15 server id 1 end_log_pos 1754 CRC32 0x7e5aecd1 Query thread_id=561 exec_time=0 error_code=0 xid=0 > > I'm sorry, I don't understand that. Could you elaborate please? I don't know, I've just shown what I observed. Some replication bug? I tried to retest and couldn't reproduce. Removed. > > > +# > > +# force_restart seems to fix this > > + > > +--source include/force_restart.inc > > + > > +# Note that test requires big enough --debug-sync-timeout unfair scheduling at high load. > > Sorry, cannot parse that. Could you elaborate please? I missed *due to* unfair scheduling at high load. That is: thread scheduling latency may reach up to 1 hour. It is executed on VM and actually VM is not always good for concurrency testing (this is quite an omission in our CI system). > > > +# slave_deadlock.opt sets this to 1 hour > > + > > +--let $rpl_topology=1->2 > > +--source include/rpl_init.inc > > + > > +--echo # > > +--echo # MDEV-30973 Enhance output of innodb deadlocks (SAMU-124, SAMU-131) > > +--echo # > > +--connection server_2 > > +SET @old_dbug= @@global.debug_dbug; > > +set @old_parallel_threads= @@global.slave_parallel_threads; > > +set @old_domain_parallel_threads= @@global.slave_domain_parallel_threads; > > +set @old_parallel_mode= @@global.slave_parallel_mode; > > +set @old_slave_transaction_retries= @@global.slave_transaction_retries; > > +set @old_innodb_lock_wait_timeout= @@global.innodb_lock_wait_timeout; > > +set @old_log_slave_retries= @@global.log_slave_retries; > > +set @old_log_slave_retries_file= @@global.log_slave_retries_file; > > +set @old_log_slave_retries_max= @@global.log_slave_retries_max; > > All this makes little sense, it's simpler to put all settings into the .opt > file > > generally it's better to do your way, to have less > server restarts, but you have an .opt file already, you can as well > use it. > > except if you decide to remove the .opt file - then nevermind, the > above it good. I'm taking it for future. > > > +--source include/stop_slave.inc > > +change master to master_use_gtid= slave_pos; > > +set global slave_parallel_threads= 10; > > +set global slave_domain_parallel_threads= 1; > > +set global slave_parallel_mode= optimistic; > > +set global slave_transaction_retries= 4; > > +set global innodb_lock_wait_timeout= 1; > ... > > diff --git a/mysql-test/suite/versioning/r/rpl_row.result b/mysql-test/suite/versioning/r/rpl_row.result > > index c161f68d0bd..6343fbecdec 100644 > > --- a/mysql-test/suite/versioning/r/rpl_row.result > > +++ b/mysql-test/suite/versioning/r/rpl_row.result > > @@ -37,12 +37,7 @@ select *, unix_timestamp(row_start) as row_start, unix_timestamp(row_end) as row > > x y row_start row_end > > 1 1 12345.000000 2147483647.999999 > > set timestamp= default; > > -### INSERT INTO `test`.`t1` > > -### SET > > -### @1=1 > > -### @2=1 > > -### @3=12345.000000 > > -### @4=2147483647.999999 > > why? That was a bug in grep.inc (fixed). I guess this commit was temporary which I forgot to deal with. > > > +/*!50530 SET @@SESSION.PSEUDO_SLAVE_MODE=1*/; > > connection slave; > > select * from t1; > > x y > > diff --git a/sql/log.cc b/sql/log.cc > > index 877315c60de..78e874fda5b 100644 > > --- a/sql/log.cc > > +++ b/sql/log.cc > > @@ -1188,9 +1188,17 @@ bool Log_to_file_event_handler:: > > log_error(enum loglevel level, const char *format, > > va_list args) > > { > > - return vprint_msg_to_log(level, format, args); > > + return vprint_msg_to_log(stderr, level, format, args); > > } > > > > +#ifdef HAVE_REPLICATION > > +bool Log_to_file_event_handler::log_slave_retry(const char *format, va_list args) > > +{ > > + DBUG_ASSERT(slave_retries_file.get()); > > + return vprint_msg_to_log(slave_retries_file.get(), NO_LEVEL, format, args); > > I don't think you should use vprint_msg_to_log and add a NO_LEVEL hack. > vprint_msg_to_log also prints into the windows event log, into > wsrep json log, and it locks LOCK_error_log. You don't need any of that, > just create your own function, it's just one fprintf() anyway. I still think formatting log message in single function is a good idea. Any other above deficiencies were fixed. > > > +} > > +#endif > > + > > void Log_to_file_event_handler::init_pthread_objects() > > { > > mysql_log.init_pthread_objects(); > > @@ -9752,13 +9787,15 @@ static void print_buffer_to_file(enum loglevel level, const char *buffer, > > } > > } > > > > +#ifndef EMBEDDED_LIBRARY > > mysql_mutex_lock(&LOCK_error_log); > > +#endif > > > > skr= my_time(0); > > localtime_r(&skr, &tm_tmp); > > start=&tm_tmp; > > > > - fprintf(stderr, "%d-%02d-%02d %2d:%02d:%02d %lu [%s] %.*s%.*s\n", > > + fprintf(file, "%d-%02d-%02d %2d:%02d:%02d [T%lu] %s%.*s%.*s\n", > > eh... no. don't change the format of existing log files, please. This change is because of a serious deficiency in logging format: you cannot grep by thread number (with simple expression) and grepping by thread number is important. I understand this may cause incompatibilities but I hope the drawbacks are less than the advantage. What if we keep it? > > > start->tm_year + 1900, > > start->tm_mon+1, > > start->tm_mday, > > @@ -9834,6 +9872,22 @@ void sql_print_error(const char *format, ...) > > } > > > > > > +#ifdef HAVE_REPLICATION > > +void sql_print_error2(const char *format, ...) > > s/sql_print_error2/print_slave_error/ ? Fixed (slave_print_error). > > > +{ > > + va_list args; > > + DBUG_ENTER("sql_print_error"); > > + > > + va_start(args, format); > > + error_log_print(ERROR_LEVEL, format, args); > > + slave_retries_print(format, args); > > + va_end(args); > > + > > + DBUG_VOID_RETURN; > > +} > > +#endif > > + > > + > > void sql_print_warning(const char *format, ...) > > { > > va_list args; > > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > > index 8cc3ad51ee9..4f6b2273597 100644 > > --- a/sql/mysqld.cc > > +++ b/sql/mysqld.cc > > @@ -765,6 +765,90 @@ char *opt_relay_logname = 0, *opt_relaylog_index_name=0; > > char *opt_logname, *opt_slow_logname, *opt_bin_logname; > > char *opt_binlog_index_name=0; > > > > +#ifdef HAVE_REPLICATION > > +char default_slave_retries_path[FN_REFLEN]; > > +bool opt_slave_retries= false; > > +char *opt_slave_retries_path= NULL; > > +uint opt_slave_retries_max_log= 0; > > +#endif > > +Slave_retries_file slave_retries_file; > > + > > +bool Slave_retries_file::open() > > +{ > > +#ifdef HAVE_REPLICATION > > + DBUG_ASSERT(opt_slave_retries); > > + DBUG_ASSERT(opt_slave_retries_path); > > + file= fopen(opt_slave_retries_path, "a"); > > my_fopen or my_open > > and anyway, why a new class, why not to use MYSQL_LOG like general and slow > logs do? I rewrote the code via MYSQL_LOG. > > > + if (!file) > > + { > > + sql_print_error("Open of log_slave_retries '%s' failed: %s (%d)", > > + opt_slave_retries_path, strerror(errno), errno); > > This is inconsistent, the server doesn't print this for other log files. > > I've just tried with general log and got > > 2023-11-06 17:58:19 0 [ERROR] mysqld: File '/tmp' not found (Errcode: 21 "Is a directory") > 2023-11-06 17:58:19 0 [ERROR] Could not use /tmp for logging (error 21). Turning logging off for the whole duration of the MariaDB server process. To turn it on again: fix the cause, shutdown the MariaDB server and restart it. > > Please, do the same Looks like wrong behavior: you supplied wrong arguments and the server was started anyway! What is the justification for this? It is easy to miss: if you don't read the error log, you don't even know something is wrong. And if you got lucky to notice that, the server is already up and running (and loaded with clients). Anyway since my logging now works like general log it now works the same. > > > + return true; > > + } > > + else > > + { > > + slave_retries_print("%s (mariadbd %s) starting as process %lu ...", > > + my_progname, server_version, (ulong) getpid()); > > Any reason, why it shouldn't be the same as in the error log? > > sql_print_information("Starting MariaDB %s source revision %s as process %lu", > server_version, SOURCE_REVISION, (ulong) getpid()); > I think it would be nice to add my_progname into the message. Made it now like in the original code. > > + } > > +#endif > > + return false; > > +} > > + > > +void Slave_retries_file::close() > > +{ > > +#ifdef HAVE_REPLICATION > > + if (file) > > + { > > + acquire(); > > + fclose(file); > > + release(); > > + } > > + file= NULL; > > +#endif > > +} > > + > > +void Slave_retries_file::init() > > +{ > > +#ifdef HAVE_REPLICATION > > + mysql_mutex_init(key_LOCK_global_system_variables, &mutex, MY_MUTEX_INIT_FAST); > > +#endif > > +} > > + > > +void Slave_retries_file::destroy() > > +{ > > +#ifdef HAVE_REPLICATION > > + mysql_mutex_destroy(&mutex); > > +#endif > > +} > > + > > +bool Slave_retries_file::acquire() > > +{ > > +#ifdef HAVE_REPLICATION > > + if (!file) > > + return false; > > + mysql_mutex_lock(&mutex); > > + return true; > > +#else > > + return false; > > +#endif > > +} > > + > > +void Slave_retries_file::release() > > +{ > > +#ifdef HAVE_REPLICATION > > + mysql_mutex_unlock(&mutex); > > +#endif > > +} > > + > > +void Slave_retries_file::flush() > > +{ > > + if (acquire()) > > + { > > + fflush(file); > > + release(); > > + } > > +} > > + > > /* Static variables */ > > > > my_bool opt_stack_trace; > > @@ -5055,6 +5145,35 @@ static int init_server_components() > > "this server. However this will be ignored as the " > > "--log-bin option is not defined."); > > } > > + > > + slave_retries_file.init(); > > + if (!log_error_file_ptr[0]) > > + fn_format(default_slave_retries_path, pidfile_name, mysql_data_home, > > + "-retries.err", MY_REPLACE_EXT); > > + else > > + fn_format(default_slave_retries_path, log_error_file_ptr, mysql_data_home, > > + "-retries.err", MY_REPLACE_EXT); > > that's not how all other log names work, > grep mysqld.cc for opt_slow_logname Now it works like other logs. > > > + if (opt_slave_retries_path && !opt_abort) > > + { > > + if (!opt_slave_retries_path[0]) > > + { > > + if (!default_slave_retries_path[0]) > > + { > > + /* fn_format failed */ > > + opt_slave_retries_path= NULL; > > + opt_slave_retries= false; > > + } > > + else > > + { > > + opt_slave_retries_path= my_strdup(key_memory_Sys_var_charptr_value, > > + default_slave_retries_path, MYF(0)); > > + } > > + } > > + > > + if (opt_slave_retries && > > + slave_retries_file.open()) > > + unireg_abort(1); > > + } > > #endif > > > > if (opt_bin_log) > > diff --git a/sql/mysqld.h b/sql/mysqld.h > > index 69cfc0cad56..5f1561ca10d 100644 > > --- a/sql/mysqld.h > > +++ b/sql/mysqld.h > > @@ -822,6 +848,8 @@ enum options_mysqld > > OPT_KEY_CACHE_CHANGED_BLOCKS_HASH_SIZE, > > OPT_LOG_BASENAME, > > OPT_LOG_ERROR, > > + OPT_SLAVE_RETRIES, > > + OPT_SLAVE_RETRIES_LOG, > > why do you need them? Removed. > > > OPT_LOWER_CASE_TABLE_NAMES, > > OPT_PLUGIN_LOAD, > > OPT_PLUGIN_LOAD_ADD, > > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > > index d742bc6c1c3..c53ca0f0097 100644 > > --- a/sql/sql_show.cc > > +++ b/sql/sql_show.cc > > @@ -10662,6 +10663,15 @@ char *thd_get_error_context_description(THD *thd, char *buffer, > > /* Don't wait if LOCK_thd_data is used as this could cause a deadlock */ > > if (!mysql_mutex_trylock(&thd->LOCK_thd_data)) > > { > > + if (thd->rgi_slave) > > is rgi_slave and its content protected by thd->LOCK_thd_data ? AFAICS they are per-thread and don't require protection. > > > + { > > + rpl_group_info *rgi= thd->rgi_slave; > > + len= my_snprintf(header, sizeof(header), > > + ", GTID %u-%u-%llu", rgi->current_gtid.domain_id, > > + rgi->current_gtid.server_id, rgi->current_gtid.seq_no); > > + str.append(header, len); > > + } > > + > > if (const char *info= thread_state_info(thd)) > > { > > str.append(' '); > > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > > index ebb64b340b0..11c321f06dc 100644 > > --- a/sql/sys_vars.cc > > +++ b/sql/sys_vars.cc > > @@ -5862,6 +5862,61 @@ Sys_slave_trans_retry_interval( > > "slave_transaction_retry_errors", > > GLOBAL_VAR(slave_trans_retry_interval), CMD_LINE(REQUIRED_ARG), > > VALID_RANGE(0, 3600), DEFAULT(0), BLOCK_SIZE(1)); > > + > > +static bool update_slave_retries_log(sys_var *self, THD* thd, enum_var_type type) > > +{ > > + if (opt_slave_retries) > > + { > > + if (!slave_retries_file.get() && slave_retries_file.open()) > > + { > > + my_error(ER_CANT_OPEN_FILE, MYF(0), opt_slave_retries_path, errno); > > + opt_slave_retries= false; > > + return true; > > + } > > + } > > + else > > + slave_retries_file.close(); > > + return false; > > +} > > + > > +static Sys_var_mybool Sys_slave_retries_log( > > + "log_slave_retries", "Log transaction retries", > > + GLOBAL_VAR(opt_slave_retries), > > + CMD_LINE(OPT_ARG, OPT_SLAVE_RETRIES), DEFAULT(false), > > + NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(0), ON_UPDATE(update_slave_retries_log)); > > + > > + > > +static bool check_slave_retries_file(sys_var *self, THD *thd, set_var *var) > > +{ > > + if (opt_slave_retries) > > + { > > + my_error(ER_NOT_SUPPORTED_YET, MYF(0), "modifying while log_slave_retries is ON"); > > as always, please don't hard-code error messages. > you can use > > my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "log_slave_retries=ON"); Fixed. > > > + return true; > > + } > > + return false; > > +} > > + > > +static bool update_slave_retries_file(sys_var *self, THD* thd, enum_var_type type) > > +{ > > + if (!opt_slave_retries_path) > > + opt_slave_retries_path= my_strdup(key_memory_Sys_var_charptr_value, > > + default_slave_retries_path, MYF(0)); > > sys_var.cc can handle that for you, see PREALLOCATED > in e.g. opt_slow_logname Yes. > > also, you don't reopen the log when its name changes, how comes? It reopens. > > > + return false; > > +} > > + > > +static Sys_var_charptr Sys_slave_retries_log_path( > > + "log_slave_retries_file", "Log file path for transaction retries", > > + GLOBAL_VAR(opt_slave_retries_path), > > + CMD_LINE(REQUIRED_ARG, OPT_SLAVE_RETRIES_LOG), DEFAULT(NULL), > > + NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(check_slave_retries_file), > > + ON_UPDATE(update_slave_retries_file)); > > + > > + > > +static Sys_var_uint Sys_slave_retries_log_max( > > + "log_slave_retries_max", "Log max N transaction retries " > > + "(default 0 means infinity)", > > just "(0 means infinity)". Default is printed separately > Fixed. > > + GLOBAL_VAR(opt_slave_retries_max_log), CMD_LINE(REQUIRED_ARG), > > + VALID_RANGE(0, UINT_MAX), DEFAULT(0), BLOCK_SIZE(1)); > > #endif > > > > static bool check_locale(sys_var *self, THD *thd, set_var *var) > > diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc > > index 0e9cef669d4..8a8eeaf39f6 100644 > > --- a/storage/innobase/lock/lock0lock.cc > > +++ b/storage/innobase/lock/lock0lock.cc > > @@ -49,6 +49,7 @@ Created 5/7/1996 Heikki Tuuri > > #include <debug_sync.h> > > > > #include <set> > > +#include <stdio.h> > > I presume Marko has reviewed (or will review) InnoDB changes I didn't receive his explicit confirmation. > > > > > #ifdef WITH_WSREP > > #include <mysql/service_wsrep.h> > > diff --git a/mysql-test/main/mysqld--help.result b/mysql-test/main/mysqld--help.result > > index 77d6d10b3e6..274c034d455 100644 > > --- a/mysql-test/main/mysqld--help.result > > +++ b/mysql-test/main/mysqld--help.result > > @@ -1673,6 +1678,9 @@ log-isam myisam.log > > log-output FILE > > log-queries-not-using-indexes FALSE > > log-short-format FALSE > > +log-slave-retries FALSE > > +log-slave-retries-file (No default value) > > this is another sign that log-slave-retries-file doesn't behave like > other log filename variables. > > > +log-slave-retries-max 0 > > log-slave-updates FALSE > > log-slow-admin-statements TRUE > > log-slow-disabled-statements sp > > Regards, > Sergei > Chief Architect, MariaDB Server > and security(a)mariadb.org Thanks! -- @midenok

1 0

Re: f22662c36b1: MDEV-33442 REPAIR TABLE corrupts UUIDs
by Sergei Golubchik 26 Feb '24

26 Feb '24

Hi, Alexander, On Feb 24, Alexander Barkov wrote: > revision-id: f22662c36b1 (mariadb-10.11.7-6-gf22662c36b1) > parent(s): 656f8867720 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2024-02-15 18:11:03 +0400 > message: > > MDEV-33442 REPAIR TABLE corrupts UUIDs Looks quite good. A question: you removed a hard-coded check for MYSQL_TYPE_NEWDECIMAL, but I don't see that you added Type_handler_olddecimal::type_handler_for_implicit_upgrade() A suggestion, rename Column_definition_implicit_upgrade(). You've changed its semantics, it's good to make sure that the code using old Column_definition_implicit_upgrade() (added e.g. in 10.6 and merged up) won't compile. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 2

Re: 7ff9e91a7d2: MDEV-32975 Default charset doesn't work with PHP MySQLi extension
by Sergei Golubchik 25 Feb '24

25 Feb '24

Hi, Alexander, the fix is good, thanks. a couple of notes about comments, see below. ok to push after fixing them On Feb 25, Alexander Barkov wrote: > revision-id: 7ff9e91a7d2 (mariadb-10.4.32-179-g7ff9e91a7d2) > parent(s): 1070575a890 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2024-01-26 13:12:03 +0400 > message: > > MDEV-32975 Default charset doesn't work with PHP MySQLi extension > > When sending the server default collation ID to the client > in the handshake packet, translate a 2-byte collation ID > to the ID of the default collation for the character set. > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 1949a0c02b6..5103ff02105 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -12831,7 +12831,32 @@ static bool send_server_handshake_packet(MPVIO_EXT *mpvio, > > int2store(end, thd->client_capabilities); > /* write server characteristics: up to 16 bytes allowed */ may be move this comment to be directly above end[2]=... line, as before? Otherwise they're separated by another comment and an if() block, so the comment looks out of place > - end[2]= (char) default_charset_info->number; > + > + /* > + Send the default server collation ID. > + Note, since MySQL-4.1 it's actually the > + client responsibility to send its character set to the server, > + so this value means nothing and is ignored by most connectors. > + But some rare connectors (e.g. PHP) still read it. where did you get it from? protocol description doesn't say that the charset field in the handshake packet is obsolete and means nothing. better to remove this claim. If you want to obsolete this field, don't do it in an obscure comment in the code, but change the protocol description in KB. > + */ > + CHARSET_INFO *handshake_cs= default_charset_info; > + if (handshake_cs->number > 0xFF) > + { > + /* > + A workaround for a 2-byte collation ID: translate it into > + the ID of the primary collation of this character set. > + */ > + CHARSET_INFO *cs= get_charset_by_csname(handshake_cs->csname, > + MY_CS_PRIMARY, MYF(MY_WME)); > + /* > + cs should not normally be NULL, however it may be possible > + with a dynamic character set incorrectly defined in Index.xml. > + For safety let's fallback to latin1 in case cs is NULL. > + */ > + handshake_cs= cs ? cs : &my_charset_latin1; > + } > + end[2]= (char) handshake_cs->number; > + > int2store(end+3, mpvio->auth_info.thd->server_status); > int2store(end+5, thd->client_capabilities >> 16); > end[7]= data_len; > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 367c87a34f8: Optimize performance of my_bitmap
by Sergei Golubchik 23 Feb '24

23 Feb '24

Hi, Monty, On Feb 23, Michael Widenius wrote: > commit 367c87a34f8 > Author: Michael Widenius <monty(a)mariadb.org> > Date: Sun Feb 18 17:30:01 2024 +0200 > > diff --git a/include/my_bitmap.h b/include/my_bitmap.h > --- a/include/my_bitmap.h > +++ b/include/my_bitmap.h > @@ -81,14 +79,15 @@ extern void bitmap_union(MY_BITMAP *map, const MY_BITMAP *map2); > extern void bitmap_xor(MY_BITMAP *map, const MY_BITMAP *map2); > extern void bitmap_invert(MY_BITMAP *map); > extern void bitmap_copy(MY_BITMAP *map, const MY_BITMAP *map2); > +extern void bitmap_copy_data(MY_BITMAP *map, const uchar *ptr, uint bits); Why? In all cases where you use it, the source is a bitmap too. you can use a function that copies MY_BITMAP to MY_BITMAP. > > extern uint bitmap_lock_set_next(MY_BITMAP *map); > extern void bitmap_lock_clear_bit(MY_BITMAP *map, uint bitmap_bit); > /* Fast, not thread safe, bitmap functions */ > -#define bitmap_buffer_size(bits) (((bits)+31)/32)*4 > +#define bitmap_buffer_size(bits) (((bits)+63)/64)*8 may be MY_ALIGN(bits, sizeof(my_bitmap_map)*8)*sizeof(my_bitmap_map) ? and the same below? less error-prone or, even better #define my_bitmap_map_bytes sizeof(my_bitmap_map) #define my_bitmap_map_bits (my_bitmap_map_bytes*8) #define bitmap_buffer_size(bits) MY_ALIGN(bits,my_bitmap_map_bits)*my_bitmap_map_bytes > #define no_bytes_in_map(map) (((map)->n_bits + 7)/8) > -#define no_words_in_map(map) (((map)->n_bits + 31)/32) > -#define bytes_word_aligned(bytes) (4*((bytes + 3)/4)) > +#define no_words_in_map(map) (((map)->n_bits + 63)/64) > +#define bytes_word_aligned(bytes) (8*((bytes + 7)/8)) > /* The following functions must be compatible with create_last_word_mask()! */ > static inline void > bitmap_set_bit(MY_BITMAP *map,uint bit) > diff --git a/mysys/my_bitmap.c b/mysys/my_bitmap.c > --- a/mysys/my_bitmap.c > +++ b/mysys/my_bitmap.c > @@ -31,95 +42,82 @@ > New version written and test program added and some changes to the interface > was made by Mikael Ronstrom 2005, with assistance of Tomas Ulin and Mats > Kindahl. > + Updated to 64 bits and use my_find_first_bit() to speed up > + bitmap_get_next_set() by Monty in 2024 > */ > > #include "mysys_priv.h" > #include <my_bitmap.h> > #include <m_string.h> > #include <my_bit.h> > +#include <my_byteorder.h> > + > + > +/* Defines to check bitmaps */ > + > +#define DBUG_ASSERT_BITMAP(M) \ > + DBUG_ASSERT((M)->bitmap); \ > + DBUG_ASSERT((M)->n_bits > 0); \ > + DBUG_ASSERT((M)->last_word_ptr == (M)->bitmap + no_words_in_map(M)-1); \ > + DBUG_ASSERT((*(M)->last_word_ptr & (M)->last_word_mask) == 0); > + > +#define DBUG_ASSERT_BITMAP_AND_BIT(M,B) \ > + DBUG_ASSERT_BITMAP(M); \ > + DBUG_ASSERT((B) < (M)->n_bits); > + > +#define DBUG_ASSERT_DIFFERENT_BITMAPS(M,N) \ > + DBUG_ASSERT_BITMAP(M); \ > + DBUG_ASSERT_BITMAP(N); > + > +#define DBUG_ASSERT_IDENTICAL_BITMAPS(M,N) \ > + DBUG_ASSERT_BITMAP(M); \ > + DBUG_ASSERT_BITMAP(N); \ > + DBUG_ASSERT((M)->n_bits == (N)->n_bits); > > /* > - Create a mask with the upper 'unused' bits set and the lower 'used' > - bits clear. The bits within each byte is stored in big-endian order. > + Create a mask for the usable bits on the LAST my_bitmap_map position for > + a bitmap with 'bits' number of bits. > + > + The lowest 'bits' bits are set to zero and the rest bits are set to 1. > + For (bits & 63) == 0 , 0 is returned as in this case all bits in > + the my_bitmap_position are significant > + > + This code assumes the underlying storage is a 64 bit ulonglong. doesn't have to be if you'll use my_bitmap_map_bits > */ > > -static inline uchar invers_last_byte_mask(uint bits) > -{ > - return last_byte_mask(bits) ^ 255; > -} > +static inline my_bitmap_map last_word_mask(uint bits) > +{ > + uint bits_in_last_map= (bits & 63); > + return bits_in_last_map ? ~((1ULL << bits_in_last_map)-1) : 0ULL; you sure a condition is faster than a linear code of few bit operations? > +} > > > +static inline my_bitmap_map first_word_mask(uint bits) > +{ > + uint bits_in_last_map= (bits & 63); > + return ~((1ULL << bits_in_last_map)-1); and no condition here. strange. and shouldn't one of these two functions be inverted? supposedly one masks all bits before the first and the other - all bits after the last. > +} > -void create_last_word_mask(MY_BITMAP *map) > -{ > - unsigned char const mask= invers_last_byte_mask(map->n_bits); > - > - /* > - The first bytes are to be set to zero since they represent real bits > - in the bitvector. The last bytes are set to 0xFF since they represent > - bytes not used by the bitvector. Finally the last byte contains bits > - as set by the mask above. > - */ > - unsigned char *ptr= (unsigned char*)&map->last_word_mask; > - > - map->last_word_ptr= map->bitmap + no_words_in_map(map)-1; > - switch (no_bytes_in_map(map) & 3) { > - case 1: > - map->last_word_mask= ~0U; > - ptr[0]= mask; > - return; > - case 2: > - map->last_word_mask= ~0U; > - ptr[0]= 0; > - ptr[1]= mask; > - return; > - case 3: > - map->last_word_mask= 0U; > - ptr[2]= mask; > - ptr[3]= 0xFFU; > - return; > - case 0: > - map->last_word_mask= 0U; > - ptr[3]= mask; > - return; > - } > -} > > > -static inline my_bitmap_map last_word_mask(uint bit) > -{ > - my_bitmap_map last_word_mask; > - uint n_bits= bit + 1; > - unsigned char const mask= invers_last_byte_mask(n_bits); > - > - /* > - The first bytes are to be set to zero since they represent real bits > - in the bitvector. The last bytes are set to 0xFF since they represent > - bytes not used by the bitvector. Finally the last byte contains bits > - as set by the mask above. > - */ > - unsigned char *ptr= (unsigned char*)&last_word_mask; > - > - switch ((n_bits + 7)/8 & 3) { > - case 1: > - last_word_mask= ~0U; > - ptr[0]= mask; > - break; > - case 2: > - last_word_mask= ~0U; > - ptr[0]= 0; > - ptr[1]= mask; > - break; > - case 3: > - last_word_mask= 0U; > - ptr[2]= mask; > - ptr[3]= 0xFFU; > - break; > - case 0: > - last_word_mask= 0U; > - ptr[3]= mask; > - break; > - } > - return last_word_mask; > -} > +/* > + Update the bitmap's last_word_ptr and last_word_mask > + Also ensure that the last world is all zero to make it > + easy to find the next set bit. > + > + Note that if n_bits is 0, then last_word_ptr will point to > + bitmap (safetly). The bitmap will not be usable for almost any operation. ether "safety" or "safely" > +*/ > + > +void create_last_word_mask(MY_BITMAP *map) > +{ > + my_bitmap_map mask= last_word_mask(map->n_bits); > + map->last_word_mask= mask; > + map->last_word_ptr= map->bitmap + MY_MAX(no_words_in_map(map),1) -1; > + if (map->n_bits > 0) > + { > + *map->last_word_ptr&= ~mask; /* Set not used bits to 0 */ > + DBUG_ASSERT_BITMAP(map); > + } > +} > > > @@ -326,37 +312,45 @@ void bitmap_set_prefix(MY_BITMAP *map, uint prefix_size) > } > if ((d= no_bytes_in_map(map)-prefix_bytes)) > memset(m, 0, d); > + DBUG_ASSERT_BITMAP(map); > } > > > +/** > + Check if bitmap is a bitmap of prefix bits set in the beginning > + > + @param map bitmap > + @param prefix_size number of bits that should be set. 0 is allowed. > + > + @return 1 Yes, prefix bits where set or prefix_size == 0. > + @return 0 No > +*/ > + > my_bool bitmap_is_prefix(const MY_BITMAP *map, uint prefix_size) > { > uint prefix_mask= last_byte_mask(prefix_size); > uchar *m= (uchar*) map->bitmap; > uchar *end_prefix= m+(prefix_size-1)/8; > uchar *end; > - DBUG_ASSERT(m); > - DBUG_ASSERT(prefix_size <= map->n_bits); > > /* Empty prefix is always true */ > if (!prefix_size) > return 1; > > + DBUG_ASSERT_BITMAP_AND_BIT(map, prefix_size-1); > + > while (m < end_prefix) > if (*m++ != 0xff) > return 0; > > - end= ((uchar*) map->bitmap) + no_bytes_in_map(map) - 1; > - if (m == end) > - return ((*m & last_byte_mask(map->n_bits)) == prefix_mask); > - > + end= ((uchar*) map->last_word_ptr) + sizeof(*map->last_word_ptr)-1; good, here you use sizeof(), not just 7. > if (*m != prefix_mask) > return 0; > > - while (++m < end) > + while (++m <= end) > if (*m != 0) > return 0; > - return ((*m & last_byte_mask(map->n_bits)) == 0); > + return 1; > } > > > @@ -447,50 +442,51 @@ void bitmap_intersect(MY_BITMAP *map, const MY_BITMAP *map2) > > /* > Check if there is some bit index between start_bit and end_bit, such that > - this is bit is set for all bitmaps in bitmap_list. > + this is atleast on bit that set for all bitmaps in bitmap_list. "at least" and "one" and "that is set" ? > > SYNOPSIS > bitmap_exists_intersection() > bitmpap_array [in] a set of MY_BITMAPs > - bitmap_count [in] number of elements in bitmpap_array > + bitmap_count [in] number of elements in bitmap_array > start_bit [in] beginning (inclusive) of the range of bits to search > end_bit [in] end (inclusive) of the range of bits to search, must be > no bigger than the bits of the shortest bitmap. > > - NOTES > - This function assumes that for at least one of the bitmaps in bitmap_array all > - bits outside the range [start_bit, end_bit] are 0. As a result is not > - necessary to take care of the bits outside the range [start_bit, end_bit]. > - > RETURN > TRUE if an intersecion exists > FALSE no intersection > */ > > -my_bool bitmap_exists_intersection(const MY_BITMAP **bitmap_array, > +my_bool bitmap_exists_intersection(MY_BITMAP **bitmap_array, > uint bitmap_count, > uint start_bit, uint end_bit) > { > uint i, j, start_idx, end_idx; > - my_bitmap_map cur_res; > + my_bitmap_map cur_res, first_map; > > DBUG_ASSERT(bitmap_count); > DBUG_ASSERT(end_bit >= start_bit); > for (j= 0; j < bitmap_count; j++) > - DBUG_ASSERT(end_bit < bitmap_array[j]->n_bits); > + { > + DBUG_ASSERT_BITMAP_AND_BIT(bitmap_array[j], end_bit); > + } > > start_idx= start_bit/8/sizeof(my_bitmap_map); > end_idx= end_bit/8/sizeof(my_bitmap_map); > > + first_map= first_word_mask(start_bit); > + cur_res= first_map; > for (i= start_idx; i < end_idx; i++) > { > - cur_res= ~0; > for (j= 0; cur_res && j < bitmap_count; j++) > cur_res &= bitmap_array[j]->bitmap[i]; > if (cur_res) > return TRUE; > + cur_res= ~(my_bitmap_map) 0; > } > - cur_res= ~last_word_mask(end_bit); > + cur_res= ~last_word_mask(end_bit+1); > + if (start_idx == end_idx) > + cur_res&= first_map; > for (j= 0; cur_res && j < bitmap_count; j++) > cur_res &= bitmap_array[j]->bitmap[end_idx]; > return cur_res != 0; > @@ -501,21 +497,26 @@ my_bool bitmap_exists_intersection(const MY_BITMAP **bitmap_array, > > my_bool bitmap_union_is_set_all(const MY_BITMAP *map1, const MY_BITMAP *map2) > { > - my_bitmap_map *m1= map1->bitmap, *m2= map2->bitmap, *end; > + my_bitmap_map *m1= map1->bitmap, *m2= map2->bitmap, *end= map1->last_word_ptr; > + DBUG_ASSERT_IDENTICAL_BITMAPS(map1,map2); > > - DBUG_ASSERT(map1->bitmap); > - DBUG_ASSERT(map2->bitmap); > - DBUG_ASSERT(map1->n_bits==map2->n_bits); > - end= map1->last_word_ptr; > while ( m1 < end) > - if ((*m1++ | *m2++) != 0xFFFFFFFF) > + if ((*m1++ | *m2++) != 0xFFFFFFFFFFFFFFFFULL) better ~(my_bitmap_map)0 > return FALSE; > /* here both maps have the same number of bits - see assert above */ > - return ((*m1 | *m2 | map1->last_word_mask) != 0xFFFFFFFF); > + return ((*m1 | *m2 | map1->last_word_mask) != 0xFFFFFFFFFFFFFFFFULL); > } > > > > +#ifdef NOT_USED > + > +/* > + If we re-introduce this one one, we should change the interface to > + use_from_bit and not from_byte. We should also add testing for this > + in bitmap-t.c > +*/ just remove it, it's easier to write it from scratch again when needed then to undertand and fix this old dead code > + > /* > Set/clear all bits above a bit. > > @@ -603,45 +601,51 @@ uint bitmap_bits_set(const MY_BITMAP *map) > my_bitmap_map *data_ptr= map->bitmap; > my_bitmap_map *end= map->last_word_ptr; > uint res= 0; > - DBUG_ASSERT(map->bitmap); > + DBUG_ASSERT_BITMAP(map); > > - for (; data_ptr < end; data_ptr++) > - res+= my_count_bits_uint32(*data_ptr); > + for (; data_ptr <= end; data_ptr++) > + res+= my_count_bits(*data_ptr); > > - /*Reset last bits to zero*/ > - res+= my_count_bits_uint32(*map->last_word_ptr & ~map->last_word_mask); > return res; > } > > void bitmap_copy(MY_BITMAP *map, const MY_BITMAP *map2) > { > - my_bitmap_map *to= map->bitmap, *from= map2->bitmap, *end; > - > - DBUG_ASSERT(map->bitmap); > - DBUG_ASSERT(map2->bitmap); > - DBUG_ASSERT(map->n_bits == map2->n_bits); > - end= map->last_word_ptr; > + my_bitmap_map *to= map->bitmap, *from= map2->bitmap, *end= map->last_word_ptr; > + DBUG_ASSERT_IDENTICAL_BITMAPS(map,map2); > > while (to <= end) > *to++ = *from++; > } > > > +/* > + Copy data into the bitmap from a byte array > +*/ > + > +void bitmap_copy_data(MY_BITMAP *map, const uchar *ptr, uint bits) not needed, as far as I understand > +{ > + my_bitmap_map *to= map->bitmap; > + uint length= (bits + 7)/8; > + uint map_length= no_bytes_in_map(map); > + DBUG_ASSERT_BITMAP_AND_BIT(map, bits-1); > + > + memcpy(to, ptr, length); > + if (map_length != length) > + bzero(to + length, map_length - length); > + *map->last_word_ptr&= ~map->last_word_mask; > +} > + > + > uint bitmap_get_first_set(const MY_BITMAP *map) > { > - uint i; > my_bitmap_map *data_ptr= map->bitmap, *end= map->last_word_ptr; > + DBUG_ASSERT_BITMAP(map); > > - DBUG_ASSERT(map->bitmap); > - > - for (i=0; data_ptr < end; data_ptr++, i++) > + for (uint i=0; data_ptr <= end; data_ptr++, i++) > if (*data_ptr) > - goto found; > - if (!(*data_ptr & ~map->last_word_mask)) > - return MY_BIT_NONE; > - > -found: > - return get_first_set(*data_ptr, i); > + return my_find_first_bit(*data_ptr) + i * sizeof(my_bitmap_map)*8; > + return MY_BIT_NONE; > } > > > @@ -703,35 +696,18 @@ uint bitmap_get_next_set(const MY_BITMAP *map, uint bitmap_bit) > > uint bitmap_get_first(const MY_BITMAP *map) this should be named bitmap_get_first_clear(). because it's always is_bit_set/is_bit_clear, is_set_all/is_clear_all, test_and_set/test_and_clear, set_bit/clear_bit, etc. there is bitmap_get_first_set, so this one must be bitmap_get_first_clear > { > - uchar *byte_ptr; > - uint i,j,k; > - my_bitmap_map *data_ptr, *end= map->last_word_ptr; > - > - DBUG_ASSERT(map->bitmap); > - data_ptr= map->bitmap; > - *map->last_word_ptr|= map->last_word_mask; > + uint i; > + my_bitmap_map *data_ptr= map->bitmap, *end= map->last_word_ptr; > + DBUG_ASSERT_BITMAP(map); > > - for (i=0; data_ptr < end; data_ptr++, i++) > - if (*data_ptr != 0xFFFFFFFF) > + for (i= 0; data_ptr < end; data_ptr++, i++) > + if (*data_ptr != 0xFFFFFFFFFFFFFFFFULL) > goto found; > - if ((*data_ptr | map->last_word_mask) == 0xFFFFFFFF) > + if ((*data_ptr | map->last_word_mask) == 0xFFFFFFFFFFFFFFFFULL) > return MY_BIT_NONE; > - > found: > - byte_ptr= (uchar*)data_ptr; > - for (j=0; ; j++, byte_ptr++) > - { > - if (*byte_ptr != 0xFF) > - { > - for (k=0; ; k++) > - { > - if (!(*byte_ptr & (1 << k))) > - return (i*32) + (j*8) + k; > - } > - } > - } > - DBUG_ASSERT(0); > - return MY_BIT_NONE; /* Impossible */ > + /* find first zero bit by reverting all bits and find first bit */ > + return my_find_first_bit(~*data_ptr) + i * sizeof(my_bitmap_map)*8; > } > > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 264aef71a37: Have ha_partition ignore HA_EXTRA..CHILDREN extra() calls if no myisamrg
by Sergei Golubchik 23 Feb '24

23 Feb '24

Hi, Michael, On Feb 23, Michael Widenius wrote: > revision-id: 264aef71a37 (mariadb-10.6.17-15-g264aef71a37) > parent(s): 042c3fc432b > author: Michael Widenius > committer: Michael Widenius > timestamp: 2024-02-22 09:20:51 +0200 > message: > > Have ha_partition ignore HA_EXTRA..CHILDREN extra() calls if no myisamrg > > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index e174ef8d219..f2b3ab3aff9 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -3030,6 +3031,10 @@ bool ha_partition::create_handlers(MEM_ROOT *mem_root) > DBUG_PRINT("info", ("InnoDB")); > m_innodb= TRUE; > } > + else if (ha_legacy_type(hton0) == DB_TYPE_MRG_MYISAM) > + { > + m_myisammrg= TRUE; > + } > DBUG_RETURN(FALSE); > } > > @@ -9419,9 +9424,14 @@ int ha_partition::extra(enum ha_extra_function operation) > } > /* Category 9) Operations only used by MERGE */ > case HA_EXTRA_ADD_CHILDREN_LIST: > + if (!m_myisammrg) > + DBUG_RETURN(0); > DBUG_RETURN(loop_partitions(extra_cb, &operation)); > case HA_EXTRA_ATTACH_CHILDREN: > { > + if (!m_myisammrg) > + DBUG_RETURN(0); > + > int result; This is, of course, quite horrible. Checking the legacy type, doing something for a specify underlying engine. But I don't see a better way of doing it, short of changing the storage engine API. What if we redefine ::extra() call to return HA_ERR_WRONG_COMMAND for unsupported extra operations? The caller will not abort on it, but it'll allow the caller to know whether the operation was executed or ignored. This will allow partitioning handler to skip all extra() methods that are not implemented in the underlying engine. For all engines and all extra operations. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: Second part of review of XA patches in bb-10.6-MDEV-31949
by Kristian Nielsen 17 Feb '24

17 Feb '24

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: > > I think it's better to define a proper API for this. > > It feels an overkill in the current case. "Define a proper API" means that you think through how this should work for different storage engines and TC_LOG implementations. And then you document those thoughts. I don't understand how doing this can be overkill, you are re-defining how durability of external XA transactions are coordinated between the server layer and the storage engines. >> And vice versa, for every xid >> X_COMPLETE found, roll it back if X is found prepared in InnoDB, > > Actually >> else commit it. > safer must be to commit both X_COMPLETE and X. Generally even if > X_COMPLETE and X commits in this order, X can race to be durable before > crash. Right, and "commit X" would mean "XA ROLLBACK X" if the "complete" operation is a rollback. I didn't follow why rolling back the X_COMPLETE would be a problem, but it's not important for now; I can see how committing should work as well. >> One reason I'm asking is I'm wondering how this will affect the scheduling >> of XA in parallel replication? Like, in case of duplicate XID, the X_PREPARE >> or X_COMPLETE could conflict between two transactions, so maybe the >> wakeup_subsequent_commits can only be done after the XA COMMIT of the >> X_PREPARE and X_COMMIT? Which I think would require changes to the current >> patch. > > Hmm, right now in the branch (see Sliding_window etc) for the duplicate > XID case, like > P_1,C_2,P_3,C_4 > P - XA-PREPARE, C - ... commit) P_3 can start only when C_2 wakes it up. Yes. So my point is: In this case, the C_2 cannot do wakeup_subsequent_commits inside the group commit along with run_commit_ordered(), that would be too early. C_2 must first complete the extra internal XA transaction on the mysql.gtid_slave_pos table before it can wakeup P_3. Or is this not a problem? > My bad I left out a mention of the plan, to treat - call it > XAC_0 -> XAP_1 > dependency - by the optimistic retry. That's in order to minimize the Aha. So if we optimistically apply the XA PREPARE, and we get an error ER_XAER_DUPID that the XID already exists as prepared, then we would do a retry like other temporary errors, is that what you mean? Is this implemented in the current patch? Maybe it is automatically handled by convert_kill_to_deadlock_error() ? If this XAC_0 -> XAP_1 conflict can be handled by normal optimistic rollback/retry, then I agree there is no need to handle the dependency explicitly in the sliding window. That was not clear to me from the code. >> This I don't understand at all. >> How can an XA PREPARE/COMMIT/ROLLBACK be marked as "ddl" ? > > That's a binlogging whim. E.g a temp table "DDL" involved into > transaction. I was not sure I could quickly eliminate the flagging, so > decided to live with it. Aha, right, CREATE TEMPORARY TABLE can be part of a transaction and thus DDL (in statement-based binlogging). I missed that. > But I could not get the meaning of > > ... handler.cc to avoid special cases for whether the binlog is enabled > The burden of checking whether the binlog is enabled, e.g for taking a > TC_LOG->log_and_order() path is on the programmer. If binlog is globally enabled (--log-bin option), then tc_log is an object of class MYSQL_BIN_LOG. Else it is class TC_LOG_MMAP (or TC_LOG_DUMMY). So a virtual function can be used instead of a conditional on opt_bin_log. But binlogging can be disabled on a transaction-by-transaction basis by setting SQL_LOG_BIN, is that the case here? Then yes, it seems an explicit condition will be needed. - Kristian.

1 0

[Andrei Elkin] Re: Third/last part of review of XA patches in bb-10.6-MDEV-31949
by andrei.elkin＠pp.inet.fi 12 Feb '24

12 Feb '24

Kristian, Using the chance to forwarding the reply that did not make to the developers list, let me comment on binlog_commit() that I might hastily rate as a big subject. > Hi Andrei, > > Here is the last part of my review of the changes in branch > bb-10.6-MDEV-31949: > >> --- a/sql/log_event_server.cc >> +++ b/sql/log_event_server.cc >> @@ -3324,6 +3324,11 @@ Gtid_log_event::Gtid_log_event(THD *thd_arg, uint64 seq_no_arg, >> flags2|= thd->lex->sql_command == SQLCOM_XA_PREPARE ? >> FL_PREPARED_XA : FL_COMPLETED_XA; >> xid.set(xid_state.get_xid()); >> + >> + // multi-engine external completion is unaware of the prepared engine #. >> + extra_engines= thd->transaction->all.ha_list ? >> + ha_count_rw_2pc(thd_arg, >> + thd_arg->in_multi_stmt_transaction_mode()) - 1 : 0; > > I cannot understand the comment. > I assume it's about that an XA COMMIT of a detached xid (different > session > that the XA PREPARE, or replication slave) - right? Correct. > > But what is the consequences of not knowing extra_engines in this > case? > Is the extra_engines used for anything than the optional truncate of > the > binlog after master crash? In the multi-engine case when the # of prepared in Engine is greater than in the value in GTID event then the trx is partially committed, so is decided for to be completed. > >> @@ -4175,7 +4173,8 @@ int XA_prepare_log_event::do_commit() >> thd->lex->xid= &xid; >> if (!one_phase) >> { >> - if ((res= thd->wait_for_prior_commit())) >> + if (thd->is_current_stmt_binlog_disabled() && >> + (res= thd->wait_for_prior_commit())) > > Why?000 > Is it to allow XA_PREPARE_LOG_EVENT to group commit with other event > groups? > That makes sense, but then shouldn't the wait_for_prior_commit() be > somewhere in handler.cc (or maybe xa.cc)? > Look at Xid_log_event::do_commit(), it doesn't have any > wait_for_prior_commit(), that is in ha_commit_one_phase(). Right. > (The wait-for-prior-commit facility is designed as a server-level > feature, > not a replication-level, it could in principle be used outside of > replication, even though it not currently (IIRC).) I think I understood what you mean which is follow the ha_commit_one_phase() pattern. [todo] > >> diff --git a/sql/sql_array.h b/sql/sql_array.h >> index 8610e971016..94c6d86ac05 100644 >> --- a/sql/sql_array.h >> +++ b/sql/sql_array.h >> @@ -172,6 +172,8 @@ template <class Elem> class Dynamic_array >> >> size_t size() const { return array.elements; } >> >> + size_t max_size() const { return array.max_element; } >> + > > Not that it hurts that much, but I think you can avoid adding this. Left over of a cleanup from the previous version. [todo] > >> --- a/sql/log.cc >> +++ b/sql/log.cc >> @@ -85,6 +85,7 @@ const char *log_bin_index= 0; >> const char *log_bin_basename= 0; >> >> MYSQL_BIN_LOG mysql_bin_log(&sync_binlog_period); >> +XID_cache_element *xid_cache_search(THD *thd, XID *xid); > > This duplicates the same declaration in sql/xa.h > So probably this can be removed now? [todo] > >> @@ -2186,16 +2202,13 @@ int binlog_commit(THD *thd, bool all, bool >> ro_1pc) >> } >> >> if (cache_mngr->trx_cache.empty() && >> - (thd->transaction->xid_state.get_state_code() != XA_PREPARED || >> - !(thd->ha_data[binlog_hton->slot].ha_info[1].is_started() && >> - thd->ha_data[binlog_hton->slot].ha_info[1].is_trx_read_write()))) >> + (!(is_preparing_xa(thd) || is_prepared_xa(thd)) || >> + !is_binlog_read_write(thd))) > > This binlog_commit function started out as the "commit" method for the > binlog handlerton. But that is not how it is used anymore, now it is > called > directly from several different places, including some places specific > to > external user XA. > > A large part of the function has become complex conditions like this > that > are basically impossible for the developer (or reviewer) to understand > exactly why this set of conditions is the correct one. Again, the > function > is trying to guess what the caller wants, depending on the state of > various > thd properties etc. > > Instead, the function should be split into multiple functions. One > that is > called to handle XA PREPARE; one for binlog_commit_by_xid(), etc. Then > each > version should be able to greatly reduce the number of conditions > needed. > > And if any logic is shared between the different versions, this can > then be > put in smaller functions that are called from each version. [my initial response was: Your proposal leads to a significant refactoring, which I add only one hunk. I'd really consider that as a separate exercise I think I've understood. Sure, binlog_prepare() that logs XA-PREPARE can call a common function with binlog_commit() that originally intended to log only normal trx:s. I can look into that. [todo] >> >> @@ -11050,10 +11124,30 @@ bool >> Recovery_context::decide_or_assess(xid_recovery_member *member, int >> round, >> } >> else if (member->in_engine_prepare < last_gtid_engines) >> { >> - DBUG_ASSERT(member->in_engine_prepare > 0); >> + DBUG_ASSERT(member->in_engine_prepare > 0 || >> + /* XA event over xid that was completed in engines */ >> + (member->xid == 0 && >> + member->xa_binlog_state >= xid_recovery_member::XA_PREPARE)); >> + if (member->in_engine_prepare > 0 && >> + member->xa_binlog_state == xid_recovery_member::XA_PREPARE) >> + { > > The function decide_or_assess() is hard to understand. > I can see that it's called in 3 cases: XID_LOG_EVENT, XA PREPARE, XA > COMMIT/ROLLBACK. > So again here, I think this should be split into 3 different > functions, one > for each case (or maybe two, one for internal XID and one for external > XA, > whatever makes the more sense). > > I don't understand the comment "XA event over xid that was completed > in > engines". But I remember seeing something in log_event_server.cc that > sets > extra_engines to 0 in the external user XA case. > > So is that the case here, that last_gtid_engines==0 marks this is user > XA? > That would be much clearer if the logic for internal and external XA > was not > mixed in the same function. > >> + /* >> + the first of the two options below can occur when round 1 turns to 2 >> + so event sequences across binlog files are like the following >> + g_current(B_0, xid), ... g_last(B^*, xid), >> + where `g`:s are xa event groups associated with the same member via >> + common `xid`. As elsewhere B_0 is the 2nd round's first and, >> + B^* the 1st round binlogs. >> + */ >> if (reset_truncate_coord(member->binlog_coord < last_gtid_coord ? >> member->binlog_coord.second : pos)) > > I'm trying to understand what is going on here. I think this is refering to > the way binlog files are scanned? > First we scan the most recent binlog file. At the end of it, we may find > that the latest binlog checkpoint is in an earlier file, then we continue to > scan those older files. So for example we may scan in order: > > master-bin.000004, master-bin.000002, master-bin.000003 > > This means that we may see the XA PREPARE and XA COMMIT out-of-order as we > go from the first scan to the remaining - "round 1 turns to 2". > > But I don't understand that this is "the first of the two options below": > > member->binlog_coord < last_gtid_coord ? member->binlog_coord.second : pos > > If last_gtid_coord is in master-bin.000002, but member->binlog_coord is in > master-bin.000004 ("round 1 turns to 2"), then I would think we would have > > member->binlog_coord > last_gtid_coord > > So it would be the *second* of the options below? What am I missing here? > > Also, I would suggest a simplification here. After we do the initial scan in > round 1, if we find that we need to scan more files, then reset all the > state and hashes. And then do a full scan of all necessary binlog files > in-order. In my example, that would be: > > master-bin.000004 > <reset state> > master-bin.000002 > master-bin.000003 > master-bin.000004 > > This way, the logic becomes simpler, as we don't need logic to handle seeing > events out-of-order. > > This will mean that occasionally we will need to scan one more binlog file, > but I think that is a good trade-off. The recovery code is notoriously > difficult to test and get correct, because it's an unusual occurence to need > recovery in the first place, and very hard to reproduce any problems. And > robust and correct recovery is the whole point of having XA in the first > palce. So making the code as simple as possible is crucial. > > The extra cost is small: > > - For one, recovery is a rare event > - Re-scanning only costs CPU, the file is already read from storage into OS > buffers. > - Normally, the binlog checkpoint completes shortly after rotation, so the > last binlog file that must be rescanned will be short. > > So simply redo the scan from the correct point when a "round 2" is needed, > to avoid any logic to handle seeing events out-of-order during recovery. > > >> @@ -11182,7 +11311,9 @@ void Recovery_context::process_gtid(int round, Gtid_log_event *gev, >> last_gtid_no2pc= false; >> last_gtid_standalone= >> (gev->flags2 & Gtid_log_event::FL_STANDALONE) ? true : false; >> - if (do_truncate && last_gtid_standalone) >> + /* a standalone XA-COMPLETE is truncation-safe */ >> + if (do_truncate && last_gtid_standalone >> + && !(gev->flags2 & Gtid_log_event::FL_COMPLETED_XA)) >> update_binlog_unsafe_coord_if_needed(linfo); > > I don't understand the comment here. > What is "a standalone XA-COMPLETE" ? > What does it mean to be "truncation-safe"? > Please reword the comment to be comprehensible. > >> + /* >> + the user XA events are decided similary to the normal transaction, >> + difference is just in the timing which is Gtid event read now. >> + */ > > I don't understand this comment. Please reformulate to make it > understandable. What does normal transactions do, and what does user > XA do? > How does the timing differ, and why? > >> + sql_print_warning("GTID %u-%u-%s XA transaction state % " >> + "in binlog file:%s pos:%llu is inconsistent " >> + "with the former same state; " >> + "consider filtered binary loggig.", >> + ctx.last_gtid.domain_id, ctx.last_gtid.server_id, >> + buf, is_xac ? "'COMPLETE'" : "'PREPARE'", >> + linfo->log_file_name, ctx.last_gtid_coord.second); > > Typo: s/loggig/logging/ > > Also, the phrase "the former same state" is not understandable (at > least to > me), what does it mean? Please reformulate. > I also have to be short now in replying to the rest, which I'll expand on tomorrow morning. Now just this. >> @@ -909,14 +910,16 @@ ha_commit_checkpoint_request(void *cookie, void (*pre_hook)(void *)) >> there's no need to rollback here as all transactions must >> be rolled back already >> */ >> -void ha_close_connection(THD* thd) >> +void ha_close_connection(THD* thd, bool skip_binlog) > > I tried to understand what the purpose of this change is. > ha_close_connection() is now called from two new places related to > replication of XA PREPARE. And in one place with this mysterious > skip_binlog=true condition. > > My guess is that this is to detach the XA PREPARE from the current > THD? The reason is described in 6cbb63f592 commit name II.1. ``` OTHER CHANGES, to ha_close_connection() are done to keep the binlog hton connection alive for the end of XA-prepare's binlogging on the parallel slave. Other than the binlog hton:s are disconnected from the slave transaction handler in TC_LOG::run_commit_ordered() which makes the prepared transaction engine objects available to binlog-commit-order followers. ``` In other words XAP_k in run_commit_ordered() releases with ha_close_connection(thd, true) all resources that the following XAC_k+i need. The latter does not need XAP_k to release binlog which it still need while being is this stack of TC_LOG::run_commit_ordered MYSQL_BIN_LOG::trx_group_commit_leader MYSQL_BIN_LOG::write_transaction_to_binlog_events MYSQL_BIN_LOG::write_transaction_to_binlog binlog_flush_cache binlog_commit_flush_xa_prepare binlog_commit binlog_prepare prepare_or_error ha_prepare > So > that it becomes visible in XA RECOVER to other threads, and (maybe > more > important) to make the THD available to run another transaction? One > of the > things InnoDB does in close_connection is: trx_disconnect_prepared(). > > But I think the purpose of the close_connection handlerton call is > different And you must have meant 'ha_close_connection', right? > - it is to free things at the end of a client connection? Well, with the ha- prefix, it's to disconnect THD from all transaction branches/htons. > So it can do > more > than detach the current external XA - which is presumably the reason > for the > need to hack it to not call the specific binlog_close_connection()? > > So this shouldn't hijack the close_connection to mean > "detach_xa_trx". It > looks like we need an explicit handlerton call for detaching an xa > transaction, that can then be used instead of this hack in > ha_close_connection(). I need to digest that in the morning. Generally I don't see a problem to conduct hton release flexibly. > >> -static my_bool xacommit_handlerton(THD *unused1, plugin_ref plugin, >> - void *arg) >> +my_bool xacommit_handlerton(THD *unused1, plugin_ref plugin, void *arg) >> { >> handlerton *hton= plugin_hton(plugin); >> - if (hton->recover) >> + if (hton->recover && hton != binlog_hton) >> { >> hton->commit_by_xid(hton, ((struct xahton_st *)arg)->xid); > >> -int ha_commit_or_rollback_by_xid(XID *xid, bool commit) >> +int ha_commit_or_rollback_by_xid(XID *xid, bool commit, THD *thd) >> { > >> + if (!skip_binlog) >> + { >> + // when binlog is ON start from its transaction branch >> if (commit) >> binlog_commit_by_xid(binlog_hton, xid); >> else >> binlog_rollback_by_xid(binlog_hton, xid); > > I think the purpose if this is to ensure that commit/rollback of XA > calls > into the binlog code first (to binlog before doing it in the engines). > > But as I think Serg also remarked already, this should not be > needed. The > binlog is the (internal) transaction coordinator when it is enabled, > we have > a dedicated interface for that, class TC_LOG. This is used elsewhere > in > handler.cc to avoid special cases for whether the binlog is enabled or > not. > > So it looks like there needs to be extra virtual functions in TC_LOG > for > the XA commit/rollback, and these can then be called on the tc_log > object > and do what is required. I need to process that tomorrow. > Then some logic in handler.cc can be omitted/simplified. > > ---- > > I think with this, I got through all of the code changes in branch > bb-10.6-MDEV-31949 as of commit > 1bdc9bc8077c79e55cb4dbfc7cc47a193d17d54f > from January 20, 2024. So this concludes the current review. I have > not > looked at the test cases in this round of review. Thanks again! Cheers, Andrei

1 1