Hi Monty, Here is my review of the patch for MDEV-34504. Looks good, just a bunch of small suggestions:
From: Monty
PURGE BINARY LOGS did not always purge binary logs. This commit fixes some of the issues and adds notifications if a binary log cannot be purged.
diff --git a/mysql-test/suite/binlog/r/binlog_index.result b/mysql-test/suite/binlog/r/binlog_index.result index 9dfda71f9a7..2d2363a7fec 100644 --- a/mysql-test/suite/binlog/r/binlog_index.result +++ b/mysql-test/suite/binlog/r/binlog_index.result @@ -30,6 +30,7 @@ flush logs; flush logs; *** must be a warning master-bin.000001 was not found *** Warnings: +Note 1375 Binary log 'master-bin.000004' is not purged because it is the current active binlog
Wording: "it is the active binlog" or "it is the currently active binlog".
+Warnings: +Note 1375 Binary log 'master-bin.000004' is not purged because it is in use by an active XID transaction
A slightly better explanation is: "because it may be needed for crash recovery". This more directly explains to the user why the file cannot be removed yet (and what happens if the user removes the file anyway through the file system). It's true that this is currently related mostly to xa transactions, but the details are more complex. It's not really "active" transactions (the transactions are already committed at this point, it's the InnoDB redo log that may not yet be synced to disk). And there may be other reasons in the future (I think maybe binlog rotation already can be a reason, too).
--- a/mysql-test/suite/binlog/t/binlog_flush_binlogs_delete_domain.test +++ b/mysql-test/suite/binlog/t/binlog_flush_binlogs_delete_domain.test @@ -91,6 +91,8 @@ while ($domain_cnt) FLUSH BINARY LOGS; --let $purge_to_binlog= query_get_value(SHOW MASTER STATUS, File, 1) --eval PURGE BINARY LOGS TO '$purge_to_binlog' +--replace_column 2 # +SHOW BINARY LOGS;
Better use --source include/show_binary_logs.inc here (for consistency with other tests, it does the same thing).
diff --git a/sql/log.cc b/sql/log.cc index c27c4f3353b..1384fb0b3e7 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -4791,8 +4791,8 @@ int MYSQL_BIN_LOG::purge_first_log(Relay_log_info* rli, bool included)
@@ -4902,7 +4903,6 @@ int MYSQL_BIN_LOG::purge_logs(const char *to_log, log_info.log_file_name); goto err; } - if (find_next_log(&log_info, 0) || exit_loop) break; }
Accidental deletion of empty line.
@@ -5428,18 +5435,23 @@ int MYSQL_BIN_LOG::real_purge_logs_by_size(ulonglong binlog_pos)
-MYSQL_BIN_LOG::can_purge_log(const char *log_file_name_arg) +MYSQL_BIN_LOG::can_purge_log(THD *thd, const char *log_file_name_arg, + bool interactive) { - THD *thd= current_thd; // May be NULL at startup
@@ -5464,8 +5479,7 @@ MYSQL_BIN_LOG::can_purge_log(const char *log_file_name_arg) purge_sending_new_binlog_file= sending_new_binlog_file; } if ((res= log_in_use(log_file_name_arg, - (is_relay_log || - (thd && thd->lex->sql_command == SQLCOM_PURGE)) ? + (is_relay_log || interactive) ? 0 : slave_connections_needed_for_purge)))
It's great that you avoid using current_thd here. In fact, with this change, thd is no longer used at all in can_purge_log(), so you don't need to pass it in as an argument any longer. (I'm guessing this is a left-over from an earlier version of the patch, before you introduced the `interactive` argument, which I also think is very good). So please remove the `thd` argument of can_purge_log() ...
@@ -5338,6 +5339,7 @@ int MYSQL_BIN_LOG::real_purge_logs_by_size(ulonglong binlog_pos) MY_STAT stat_area; char to_log[FN_REFLEN]; ulonglong found_space= 0; + THD *thd= current_thd;
... and then you can also remove this call of current_thd.
@@ -5473,9 +5487,39 @@ MYSQL_BIN_LOG::can_purge_log(const char *log_file_name_arg) waiting_for_slave_to_change_binlog= 1; strmake(purge_binlog_name, log_file_name_arg, sizeof(purge_binlog_name)-1); + if (res == 1) + reason= "it is in use by a slave thread"; + else + reason= "less than 'slave_connections_needed_for_purge' slaves has " + "processed it"; + goto error;
Grammar: s/has/have/: "less than 'slave_connections_needed_for_purge' slaves have processed it"
@@ -5847,7 +5850,28 @@ int mysqld_main(int argc, char **argv) #ifdef WITH_WSREP wsrep_set_wsrep_on(nullptr); if (WSREP_ON && wsrep_check_opts()) unireg_abort(1); -#endif + + if (!opt_bootstrap && WSREP_PROVIDER_EXISTS && WSREP_ON) + { + if (global_system_variables.binlog_format != BINLOG_FORMAT_ROW) + { + sql_print_warning("Binlog_format changed to \"ROW\" because of Galera"); + global_system_variables.binlog_format= BINLOG_FORMAT_ROW; + mark_binlog_format_used(binlog_format_used); + }
This change seems unrelated to MDEV-34504. Not critical I think, but for the future it's best to put such changes in a separate commit. (The change itself is good, I think.)
@@ -1303,13 +1312,19 @@ Sys_slave_connections_needed_for_purge( "slave_connections_needed_for_purge", "Minimum number of connected slaves required for automatic binary " "log purge with max_binlog_total_size, binlog_expire_logs_seconds " - "or binlog_expire_logs_days.", + "or binlog_expire_logs_days. Default is 0 when Galera is enabled and 1 " + "otherwise.",
Since Galera is #ifdef, I guess this should be something like: "or binlog_expire_logs_days. " #ifdef WSREP "Default is 0 when Galera is enabled and 1 otherwise.", #else "Defaults to 1.", #endif
diff --git a/sql/sys_vars.h b/sql/sys_vars.h new file mode 100644 index 00000000000..8f4eac38cd0 --- /dev/null +++ b/sql/sys_vars.h @@ -0,0 +1,17 @@ +/* Copyright (c) 2024, MariaDB Corporation. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License.
Hm. It's not copyrighted by MariaDB Corporation solely, there are other copyright holders. If you want to have a copyright line before the GPL header, you could maybe say "Copyright (c) 2024, MariaDB Corporation and others", or "Parts of this file Copyright (c) 2024, MariaDB Corporation". Or just omit it. - Kristian.