MDEV-28302 Configurable `DEFAULT`s for CHANGE MASTER #4430

ParadoxV5 · 2025-11-09T04:55:38Z

The Jira issue number for this PR is: MDEV-28302
- The Jira issue number for this PR is: MDEV-37530

Description

An amazing description should answer some questions like:

What problem is the patch trying to solve?
If some output changed that is not visible in a test case, what was it looking like before the change and how it's looking with this patch applied?
Do you think this patch might introduce side-effects in other parts of the server?

1. MDEV-37530 Refactor Master & Relay Log info to iterable tuples

As this refactor will disconnect it from fixes for some open bugs in prior versions, this commit also:

Fixes MDEV-38010 number parsing ignores trailing garbage and overflows
Supercedes MDEV-38020 integer overflow
Almost fixes MDEV-35879 Slave_heartbeat_period is imprecise
Prepares to make master_retry_count 64-bit on LLP64 (e.g., Windows) to match LP64 (e.g., Linux)

Release Notes

TODO: What should the release notes say about this change?
Include any changed system variables, status variables or behaviour. Optionally list any https://mariadb.com/kb/ pages that need changing.

How can this PR be tested?

TODO: modify the automated test suite to verify that the PR causes MariaDB to behave as intended.
Consult the documentation on "Writing good test cases".

If the changes are not amenable to automated testing, please explain why not and carefully describe how to test manually.

PR quality check

This is a new feature or and a refactoring, and the PR is based against the main branch.
~~This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.~~
- While the MDEV-37530 covers a few bugs, its fixes are not suitable for versions before its refactor.
I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

These iterable tuples build towards refactoring Master & Relay Log info with DRY loops – their persistence for MDEV-37530, and possibly SHOW REPLICA STATUS and even CHANGE MASTER in the future.

Link `MasterInfoFile` & `RelayLogInfoFile` structs by making corresponding `Master_info` & `Relay_log_info` properties references to the fields. Properties are not moved for now due to the large number of changes. Also adjust usages affected by the addition of fields with `DEFAULT`s in preparation for MDEV-28302 “CHANGE MASTER TO …=DEFAULT”, except for persistence code that will be separately adjusted for MDEV-37362.

This commit finishes expanding MDEV-28302 (WIP)’s `DEFAULT`-capable, loop-based implementation of Master & Relay Log info file persistence. This conclusion realizes MDEV-37530’s goal. This involves moving – and improving, while here – the string and ID array parsers to `InfoFile::Persistent::load_from()` implementations. They and the other overrides supercede the `init_*var_from_file()` helpers and are will not overflow `int`s as reported by MDEV-38020. Also while here, fix MDEV-38010 “trailing garbage does not error”, as the MDEV-37530 refactors will disconnect them from the patch for prior versions. On the topic of transitioning to iterable fields interface, this commit also removes these functions: * `Domain_id_filter::init_ids()`: unused starting from this commit * `Domain_id_filter::store_ids(THD *)` & `prot_store_ids()`: unused since the Information Schema reimplemented SHOW REPLICA STATUS. This commit also has a few minor adjustments and fixes for the previous two commits.

I don’t mind highlighting them; but for records’ sake, TODO: squash

ParadoxV5

Large projects come with large discussions, ~~like power and responsibility~~.

ParadoxV5 · 2025-11-09T05:03:33Z

sql/rpl_info_file.hh

Would this .hh extension be suitable for distinguishing “single-header libraries” (and C++20 modules) from our traditional h-cc pairs?

We already have single-header libraries that still use the regular .h (e.g. sql_sort.h, was just the first I found). I think it is ok to keep just .h, otherwise we'd have differing conventions.

ParadoxV5 · 2025-11-09T05:04:50Z

sql/rpl_relay_log_info_file.hh

+  /// Relay_Master_Log_File (of the event *group*)
+  StringField<> read_master_log_file;
+  /// Exec_Master_Log_Pos (of the event *group*)
+  IntField<my_off_t> read_master_log_pos;


If (hypothetically) we add the IO thread’s Relay Log position (i.e., Gtid_IO_Pos but for file-position mode) to CM/SSS, what would we name it?

ParadoxV5 · 2025-11-09T05:08:54Z

sql/rpl_info_file.hh

+
+
+  /// Persistence interface for an unspecified item
+  struct Persistent


I thought about what CHANGE MASTER would look like as a table that supports SQL, such as SELECT and INSERT.
I decided that these new structs are alright, since a system table does not permit DDL (e.g., ALTER TABLE).

I am mainly concerned about an “unnecessary performance tax” for using a full-blown storage engine such as Aria, MyISAM or Memory, particularly their ACID compliance mechanisms.

Speaking of system tables, perhaps some of them can utilize a “system” storage engine, too, where storage is C++-first and DDL is not supported.

SHOW REPLICA STATUS could use a virtual method in this class, too; I am not sure.

After all, we still need to write out info files, at least on demand, if we support downgrades.
What’s the point of their forward compatibility, after all?

ParadoxV5 · 2025-11-09T05:10:35Z

sql/rpl_master_info_file.hh

+/// }@
+
+
+struct MasterInfoFile: InfoFile


Name conflict, I know.
But, Master_info & Relay_log_info also appear to contain runtime data for IO and SQL threads, respectively.
This inadequacy in organization makes maintenance difficult to know what belongs to whom, not to mention one of the 0b10 hardest problems in software development.

ParadoxV5 · 2025-11-09T05:11:10Z

sql/rpl_mi.cc

-
-    if (init_intvar_from_file(&master_log_pos, &mi->file, 4) ||
-        init_strvar_from_file(mi->host, sizeof(mi->host), &mi->file, 0) ||
-        init_strvar_from_file(mi->user, sizeof(mi->user), &mi->file, "test") ||


ParadoxV5 · 2025-11-09T05:28:50Z

sql/rpl_master_info_file.hh

+          if (c == my_b_EOF)
+            return true;
+          buf[i]= c;
+          if (c == /* End of Count */ ' ' || c == /* End of Line */ '\n')


TODO: extend my_b_gets() to accept multiple delimiters

ParadoxV5 · 2025-11-09T05:30:13Z

libmysqld/CMakeLists.txt

           ../sql/filesort_utils.cc ../sql/sql_digest.cc
           ../sql/filesort.cc ../sql/grant.cc
-           ../sql/gstream.cc ../sql/slave.cc
+           ../sql/gstream.cc


when moving these functions to mysys, don't forget to remove slave.cc from libmysqld/CMakeLists.txt
⸺ init_intvar_from_file()’s code documentation

ParadoxV5 · 2025-11-09T05:44:33Z

sql/rpl_master_info_file.hh

+          gtid_supported ?
+            enum_master_use_gtid::SLAVE_POS : enum_master_use_gtid::NO


From the code, it looks like if the primary is before MariaDB 10 (back when GTIDs were not available), the replica sets its master_use_gtid to NO and remembers this as its new default for RESET REPLICA.
IMO, it would be nice if the replica would automatically default back to Slave_Pos if the connection changes to a MariaDB 10+ primary.

Here, this “remember NO as its new DEFAULT” does not override the option-configured default, since the primary’s version is not known until the replica (re)connects to it.
Should it?

Or, maybe we should silently fall back to NO without affecting this config.

Currently, if the primary is MariaDB 10+, RESET REPLICA also warns about changing Using_Gtid (sic) if master_use_gtid isn’t already Slave_Pos.

In fact, how old a MariaDB/MySQL version do we maintain the backward compatibility for anyway?

ParadoxV5 · 2025-11-09T05:47:47Z

sql/rpl_master_info_file.hh

Look, @DaveGosselin-MariaDB, C++17!

ParadoxV5 · 2025-11-09T05:55:43Z

storage/perfschema/table_replication_connection_configuration.cc

+  m_row.ssl_allowed= mi->master_ssl ?
+  #ifdef HAVE_OPENSSL
+    PS_SSL_ALLOWED_YES
+  #else
+    PS_SSL_ALLOWED_IGNORED
+  #endif
+  : PS_SSL_ALLOWED_NO;


The PerfSchema has its own copies of enum_rpl_yes_no, enum_ssl_allowed and enum_using_gtid, but with each value one more than the corresponding Master_info enum.
It could be that the enums in the PerfSchema storage engine start from 1 instead of 0.

What should we do about them?
Not to mention the rest of the Replica Status fields duplicated between the InfoSchema and the PerfSchema.

ParadoxV5

I am gonna take a short break from this project.

Known issues with `master_heartbeat_period` in the draft so far

It was reset not with RESET REPLICA but with CHANGE MASTER.
And now I removed this unintuitive CHANGE MASTER reset, but forgot to add it to RESET REPLICA.
MDEV-35879 Slave_heartbeat_period is imprecise:
I am getting 4294967.040. (We might actually make this a blocker.)

ParadoxV5 · 2025-11-10T05:35:19Z

sql/rpl_master_info_file.hh

+    #ifdef __clang__
+      // FIXME: floating-point variants of `std::from_chars()` not supported


According to this Stack Overflow answer, libc++ (Clang) doesn’t support floating-point variants of std::from_chars().
How did it pass our (relatively recent) C++17 requirement, then?

bnestere

Hi @ParadoxV5 !

I gave the patch a read-through. Not super in-depth b/c it is still a draft (thanks for the early publish!), but enough to get the idea, and I think it is good so far. I left some notes.

A general concern I have is that it is effectively un-loop-unrolling and adding in additional function calls, and I'd worry it would make MDEV-37684 worse. So I think we should run some benchmarks with this to make sure it isn't incurring any additional overhead.

bnestere · 2025-11-11T22:59:25Z

sql/rpl_info_file.hh

+  [C++20 modules](https://en.cppreference.com/w/cpp/language/modules.html)
+  can supercede headers and their `#include` guards.
+*/
+struct InfoFile


The naming conventions don't follow the replication general pattern of Uppercase_lowercase_underscore_splits. We should try to be consistent.

Acknowledged.
Though, what do you mean by a replication general pattern? Are replication patterns diverted from the rest of the server?

Well, our official MariaDB guidelines say:

New types should be named in Capital_snake_case. You can consider using UPPER_SNAKE_CASE rarely, when consistency with old code base is stylistically important.
Older st_prefixed_snake_case should be never used in the new code.

But I've noticed other parts of the code sometimes have their own styles. The general rule is that new code in an existing file should follow the conventions already established in that file (so the file itself is consistent), and then new files should adhere to our official standards.

Well, our official MariaDB guidelines say:

Ah, CODING_STANDARDS.md.
I totally forgot about it with my first new classes for MariaDB 😅.

But I've noticed other parts of the code sometimes have their own styles.

Either them or that standard needs to explain the histories…

bnestere · 2025-11-11T23:00:26Z

sql/rpl_master_info_file.hh

+#include "rpl_info_file.hh"
+#include <unordered_map> // Type of @ref MasterInfoFile::FIELDS_MAP
+#include <string_view>   // Key type of @ref MasterInfoFile::FIELDS_MAP
+#include <unordered_set> // Parameter type of ChangeMaster::set_defaults() //?


Is this needed? I only saw seen of that type, and I'm not certain is is effectively used anymore (perhaps it was needed in an earlier version?)

Aye, that comment was a leftover from an earlier MDEV-28302 design that I expect to replace.

bnestere · 2025-11-11T23:01:22Z

sql/rpl_master_info_file.hh

+    bool load_from(IO_CACHE *file) override
+    {
+      buf[1]= false; // not default
+      return StringField<>::load_from(file);


This overwrites buf, yet you use buf to track "defaultness". That can make things hard to debug, I'd suggest to just have a variable to track default.

bnestere · 2025-11-11T23:03:12Z

sql/rpl_master_info_file.hh

+    calculated as an unsigned integer milliseconds field.
+    It has a `DEFAULT` value of @ref ::master_heartbeat_period,
+    which in turn has a `DEFAULT` value of `@@slave_net_timeout / 2` seconds.
+  */


Where you define the struct body of a variable at the same time as its declaration, it'd be good to make it clear in the comment what the following definition is before, so one doesn't have to go down a bunch of lines to find it (applies to everywhere you have this pattern).

bnestere · 2025-11-11T23:03:49Z

sql/rpl_master_info_file.hh

+  static constexpr const char END_MARKER[]= "END_MARKER";
+  /// An iterable for the `key=value` section of `@@master_info_file`
+  // C++ default allocator to match that `mysql_execute_command()` uses `new`
+  inline static const std::unordered_map<std::string_view, mem_fn> FIELDS_MAP= {


Why unordered? As the FIELDS_MAP is iterated over when saving the KV pairs, wouldn't that make it non-deterministic? The fields should be written in the same order as pre-MDEV-37530 so users can downgrade without breaking anything.

The pre-MDEV-37530 code checks keys, too, so the order is not important.
I can see that the predecessor who added this key-value section in the first place had order independence in mind.

(That code is a chain of if (got_eq && !seen_X && !strcmp(buf, "X")), however, which will look ridiculous under my (backward-compatible!) plan of stuffing the DEFAULT choices in that section, not to mention performance comparisons with hash lookups.)

bnestere · 2025-11-11T23:04:27Z

sql/rpl_master_info_file.hh

+      MariaDB 10.0 does not have the `END_MARKER` before any left-overs at
+      the end of the file, so ignore any non-first occurrences of a key.
+    */
+    auto seen= std::unordered_set<const char *>();


What is this used for? I see it is inserted into, but never used after that.

else if (seen.insert(key).second) // if no previous insertion

The second member of the return of std::unordered_set::insert(value_type&&) is true if and only if the set doesn’t already have the element.

I’ll expand the comment to describe this API.

Ah right, my blunder 🤦. Though with that said, we should prefer MariaDB data types (when available) to promote a standard way of doing things. Here, you should be able to just use HASH (and it should be used in the same way as sql_hset.h's Hash_set, though I'm not sure if you can just use Hash_set because you're just using const char *.).

And another question - do we need to track seen? Would it otherwise just overwrite the last value seen (where there shouldn't be duplicates anyway)? That's how config options in my.cnf work, and I'd think we can keep that same behavior.

Here, you should be able to just use HASH (and it should be used in the same way as sql_hset.h's Hash_set, though I'm not sure if you can just use Hash_set because you're just using const char *.).

Indeed, though I prefer the C++ standard library to roll fewer things of my own.
Here, I can use Hash_set, but I would either have to specify a charset (for comparing contents) or a comparator of (constant) pointers by their face values.

I am actually looking forward to a future where we use PSI_memory_key-configured C++ allocators with the best C++ standard library implementation.

Would it otherwise just overwrite the last value seen (where there shouldn't be duplicates anyway)? That's how config options in my.cnf work, and I'd think we can keep that same behavior.

I asked this question too.

The “code document” of seen is slightly edited from the comment at the corresponding section in rpl_mi.cc, which was:

10.0 does not have the END_MARKER before any left-overs at the end
of the file. So ignore any but the first occurrence of a key.

That is, a file from 10.0 could have two entries of a key by chance and intend the first to be the real one.

(Of course, you could also argue that 10.0 is long EOL, nobody should have them around, and we can get rid of code that holds those compatibilities together.)

bnestere · 2025-11-11T23:04:57Z

sql/rpl_master_info_file.hh

+          key[i]= c;
+        }
+      }
+      break_for:;


labels shouldn't be indented

bnestere · 2025-11-11T23:10:33Z

sql/rpl_master_info_file.hh

+  /// }@
+
+
+  inline static const std::initializer_list<mem_fn> FIELDS_LIST= {


Can this be combined with FIELDS_MAP?

I thought about that once.
But nearly all of the MDEV-28302 fields are in the line-by-line section (FIELDS_LIST), and I want to write those fields both in this old-style section to preserve the defaulted values for downgrades, and in the key-value section to indicate the DEFAULT state.
The differences in the two sections encouraged me to simply “unwind this loop”.

Developing forward compatibility for a feature in older versions (where only the Enterprise Server receive features) sure is a task.

ParadoxV5 added 5 commits November 3, 2025 20:22

MDEV-37530: Create iterables for Repl. info files

2f8cea4

These iterable tuples build towards refactoring Master & Relay Log info with DRY loops – their persistence for MDEV-37530, and possibly SHOW REPLICA STATUS and even CHANGE MASTER in the future.

Merge branch 'main' into MDEV-37530

9722e56

Silly but deadly mistakes caught by MTR!

7a8834b

I don’t mind highlighting them; but for records’ sake, TODO: squash

ParadoxV5 requested review from andrelkin and bnestere November 9, 2025 04:55

ParadoxV5 added MariaDB Corporation Replication Patches involved in replication labels Nov 9, 2025

ParadoxV5 changed the title ~~MDEV-28302 configurable DEFAULTs for CHANGE MASTER~~ MDEV-28302 Configurable DEFAULTs for CHANGE MASTER Nov 9, 2025

ParadoxV5 commented Nov 9, 2025

View reviewed changes

issues found by CI, 1st batch

3ba73e6

ParadoxV5 commented Nov 10, 2025

View reviewed changes

bnestere requested changes Nov 11, 2025

View reviewed changes



		/// Persistence interface for an unspecified item
		struct Persistent

		gtid_supported ?
		enum_master_use_gtid::SLAVE_POS : enum_master_use_gtid::NO

		#ifdef __clang__
		// FIXME: floating-point variants of `std::from_chars()` not supported

		/// }@


		inline static const std::initializer_list<mem_fn> FIELDS_LIST= {

Uh oh!

MDEV-28302 Configurable DEFAULTs for CHANGE MASTER #4430

Are you sure you want to change the base?

MDEV-28302 Configurable DEFAULTs for CHANGE MASTER #4430

Uh oh!

Conversation

ParadoxV5 commented Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

1. MDEV-37530 Refactor Master & Relay Log info to iterable tuples

Release Notes

How can this PR be tested?

PR quality check

Uh oh!

ParadoxV5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ParadoxV5 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Known issues with master_heartbeat_period in the draft so far

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnestere left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnestere Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

MDEV-28302 Configurable `DEFAULT`s for CHANGE MASTER #4430

MDEV-28302 Configurable `DEFAULT`s for CHANGE MASTER #4430

ParadoxV5 commented Nov 9, 2025 •

edited

Loading

ParadoxV5 left a comment •

edited

Loading

Known issues with `master_heartbeat_period` in the draft so far

bnestere Nov 12, 2025 •

edited

Loading