MDEV-38849 slave_connections_needed_for_purge prevents independent machine from purging binary logs - #5559
MDEV-38849 slave_connections_needed_for_purge prevents independent machine from purging binary logs#5559ParadoxV5 wants to merge 1 commit into
Conversation
…chine from purging binary logs `@@slave_connections_needed_for_purge`’s default of `1` ensures binary log availability on replication masters, but is not a sensible default suitable for all scenarios, especially for long-term slave servers and standalone (not in a replication setup) servers. The outcome was that standalone server users were confused why automatic binlog purging does not work. This commit changes this default to `0`, which is suitable for both standalone and (when backed by prompt failure recovery) replication setups. `0` also more closely matches the behaviour before MDEV-31404, which added this variable, out of the box. This commit also adds a one-time replication warning when registering a slave, but `@@slave_connections_needed_for_purge` is left unchanged. Rather than enforcing a defence with an unsensible default, this reminder will bring awareness of the risk of automatic binlog purging. This commit also cleans up Galera and MTR workarounds to the introduction of the `@@slave_connections_needed_for_purge=1` default.
ParadoxV5
left a comment
There was a problem hiding this comment.
The main problem is that users don't know the parameter exists […]. Also, a user may only want a value of 1 to ensure at least one slave is connected before purging, but may have many slaves connected - a user wouldn't want a warning here.
⸺ @bnestere, JIRA comment
There was a problem hiding this comment.
Implementation is tricky because we don’t want to warn multiple times when the only slave disconnects and reconnects.
My preferred solution is using a std::atomic<bool>.
It starts with “do warn” and becomes “don’t warn” upon warning or when @@slave_connections_needed_for_purge is changed.
However, “when changed” turned out to be impractical to implement, for Server Options and System Variables don’t actually share code; specifically, Options do not call Variables’ ON_CHECK/ON_UPDATE callbacks.
Instead, as shown in mysqld.cc and sys_vars.cc, there must be additional copies of code to cover the Options.
Although this design does offer flexibility, such as how options don’t need to worry about synchronizing user threads, it is still inherently error-prone and has led me on a wild goose chase.
I also had the alternative of a call-once block, implemented with the initialization of a function-local static.
- ➕ In the absence of infrastructure improvements, abandoning changing a state in Options/Variables leaves this site as the only accessor of this
std::atomic<bool>, so a self-contained solution is suitable.- ➕ It does not even require this explicit
std::atomic<bool>anymore, since the compiler will include one.
- ➕ It does not even require this explicit
- Its main downside on paper is the lacklustre expressiveness,
but not like our ancient code is much more expressive either. - ➖ In practice, though, I don’t have another way to quickly distinguish whether the variable is left as default.
The statistic would besys_var::value_origin, butsys_varrequires mutex synchronization, which is whyIS_SYSVAR_AUTOSIZE()is banned after server startup.
| if (variables.log_warnings >= 1 && // Verbosity Level "Binlog/Replication" | ||
| warn_slaves_not_needed_for_purge.exchange(false, | ||
| // no need to sync with `@@slave_connections_needed_for_purge` itself | ||
| std::memory_order_relaxed)) |
There was a problem hiding this comment.
std::memory_order_relaxed should be sufficient here, since the only ordering that relates to this flag is the log output.
&&is not reörderable, rïght?- This procedure doesn’t need to care about whether
@@slave_connections_needed_for_purgeis actually still 0.
As the variable currently stands, it’d need mutex synchronization to care, too.
| sql_print_information( | ||
| "slave_connections_needed_for_purge changed to 0 because " | ||
| "of Galera. Change it to 1 or higher if this Galera node " | ||
| "is also Master in a normal replication setup"); |
There was a problem hiding this comment.
Should Galera slaves not count towards “when registering a slave”?
If not, what’s the definitive way to distinguish Galera mode?
@@slave_connections_needed_for_purge’s default of1ensures binary log availability on replication masters, but is not a sensible default suitable for all scenarios, especially for long-term slave servers and standalone (not in a replication setup) servers.The outcome was that standalone server users were confused why automatic binlog purging does not work.
This commit changes this default to
0, which is suitable for both standalone and (when backed by prompt failure recovery) replication setups.0also more closely matches the behaviour before MDEV-31404, which added this variable, out of the box.This commit also adds a one-time replication warning when registering a slave, but
@@slave_connections_needed_for_purgeis left unchanged.Rather than enforcing a defence with an unsensible default, this reminder will bring awareness of the risk of automatic binlog purging.
This commit also cleans up Galera and MTR workarounds to the introduction of the
@@slave_connections_needed_for_purge=1default.