Hi Monty,
Here is my review of the patch for MDEV-34504. Looks good, just a bunch of
small suggestions:
> From: Monty <mo...@mariadb.org>
>
> 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.
_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-le...@lists.mariadb.org
