MDEV-28302 Configurable DEFAULTs for CHANGE MASTER
#4430
Conversation
ParadoxV5
changed the title
DEFAULTs for CHANGE MASTER
ParadoxV5
left a comment
ParadoxV5
left a comment
There was a problem hiding this comment.
Large projects come with large discussions, like power and responsibility.
sql/rpl_relay_log_info_file.hh
Outdated
| /// 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; |
There was a problem hiding this comment.
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?
|
|
||
|
|
||
| /// Persistence interface for an unspecified item | ||
| struct Persistent |
There was a problem hiding this comment.
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?
sql/rpl_master_info_file.hh
Outdated
| /// }@ | ||
|
|
||
|
|
||
| struct MasterInfoFile: InfoFile |
There was a problem hiding this comment.
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.
|
|
||
| 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") || |
| if (c == my_b_EOF) | ||
| return true; | ||
| buf[i]= c; | ||
| if (c == /* End of Count */ ' ' || c == /* End of Line */ '\n') |
There was a problem hiding this comment.
TODO: extend my_b_gets() to accept multiple delimiters
| ../sql/filesort_utils.cc ../sql/sql_digest.cc | ||
| ../sql/filesort.cc ../sql/grant.cc | ||
| ../sql/gstream.cc ../sql/slave.cc | ||
| ../sql/gstream.cc |
There was a problem hiding this comment.
when moving these functions to mysys, don't forget to remove slave.cc from libmysqld/CMakeLists.txt
⸺init_intvar_from_file()’s code documentation
| gtid_supported ? | ||
| enum_master_use_gtid::SLAVE_POS : enum_master_use_gtid::NO |
There was a problem hiding this comment.
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
NOas its newDEFAULT” 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
NOwithout affecting this config.- Currently, if the primary is MariaDB 10+,
RESET REPLICAalso warns about changingUsing_Gtid(sic) ifmaster_use_gtidisn’t alreadySlave_Pos.
- Currently, if the primary is MariaDB 10+,
- In fact, how old a MariaDB/MySQL version do we maintain the backward compatibility for anyway?
| m_row.ssl_allowed= mi->master_ssl ? | ||
| #ifdef HAVE_OPENSSL | ||
| PS_SSL_ALLOWED_YES | ||
| #else | ||
| PS_SSL_ALLOWED_IGNORED | ||
| #endif | ||
| : PS_SSL_ALLOWED_NO; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 getting4294967.040. (We might actually make this a blocker.)
bnestere
left a comment
bnestere
left a comment
There was a problem hiding this comment.
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.
sql/rpl_master_info_file.hh
Outdated
| bool load_from(IO_CACHE *file) override | ||
| { | ||
| buf[1]= false; // not default | ||
| return StringField<>::load_from(file); |
There was a problem hiding this comment.
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.
sql/rpl_master_info_file.hh
Outdated
| 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= { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
| 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 *>(); |
There was a problem hiding this comment.
What is this used for? I see it is inserted into, but never used after that.
There was a problem hiding this comment.
else if (seen.insert(key).second) // if no previous insertionThe 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Here, you should be able to just use
HASH(and it should be used in the same way assql_hset.h'sHash_set, though I'm not sure if you can just useHash_setbecause you're just usingconst 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.cnfwork, 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.)
There was a problem hiding this comment.
Oops, I missed that you'd replied. I'd asked Monty about the supplying our own C++ allocators to std functions, to which he'd said
No work ever planned for allocator implementation. The issue is that for most cases MariaDB string functions are superior to std:: versions (performance, support of all MariaDB character sets, usage of mem_root etc). In general I want people to use our functions instead of std:: to not get code bloat and keep a standard way of doing things.
So I'd still say we should use our own Hash_set here.
There was a problem hiding this comment.
No work ever planned for allocator implementation.
I understand: No work ever planned for refactors like this one at all 🤷.
The issue is that for most cases MariaDB string functions are superior to std:: versions
This specific instance doesn’t actually require comparing string contents, only the pointed addresses. I’ll include a note, too.
performance
Interesting; a benchmark would show a critical comparison.
support of all MariaDB character sets
Of course.
Charset support is its own layer regardless of the underlying structure – char[], LEX_(C)STRING/std::string_view, or std::string.
usage of mem_root
My allocators draft has one for MEM_ROOT too.
In general I want people to use our functions instead of std:: to not get code bloat and keep a standard way of doing things.
So I'd still say we should use our own
Hash_sethere.
I completely agree: We either stick with our current library or switch in a few fell swoops, be it collections or another component (e.g., JSON).
The standard way would extend to the field value lists, though, and I will no longer have list initializers unless I “port them from std::”…
sql/rpl_master_info_file.h
Outdated
| bool load_from_file() override | ||
| { | ||
| /// Repurpose the trailing `\0` spot to prepare for the `=` or `\n` | ||
| static constexpr size_t MAX_KEY_SIZE= sizeof("ssl_verify_server_cert"); |
There was a problem hiding this comment.
Syntax error on Windows ARM64.
Google results say that it is because MAX_KEY_SIZE is an existing macro (defined outside of our repository).
ParadoxV5
left a comment
ParadoxV5
left a comment
There was a problem hiding this comment.
buildbot/amd64-msan-clang-20 has been consistently segfaulting during server startup in the recent snapshots.
I am unable to tell from the logs and the first crashing snapshot’s logs; I would like your assistance.
Besides this problem and commit squashing, the project is ready for review, @bnestere & @andrelkin.
| sprintf(static_cast<char*>(buff), "%.3lf", | ||
| mi->master_heartbeat_period/1000.0); |
There was a problem hiding this comment.
Hm, maybe the cancelled master_heartbeat_period → string method still has one (and only one) use.
Though the odd truth is that status variables are stuck with all values being strings, even though many (if not the majority) are numeric.
| mi->master_use_gtid.set_default(); | ||
| mi->master_heartbeat_period.set_default(); |
There was a problem hiding this comment.
There was no “USE_GTID_DEFAULT” previously, so master_use_gtid was reset only conditionally.
master_heartbeat_period, OTOH, was reset in CHANGE MASTER rather than RESET REPLICA.
I wonder, though, why doesn’t RESET REPLICA ALL reset all CHANGE MASTER fields of the default unnamed connection, whereas it is a complete deletion for named (as in multi-source) connections?
There was a problem hiding this comment.
A question for the future (no need to answer immediately):
Should other fields (the ones not listed) “recognize” the DEFAULT keyword as well?
Fields with trivial DEFAULTs
- [P.S.] source name (the filename of the info files): Not specifying a name already means
DEFAULT, which is@@SESSION.default_master_connection, whose default is empty. - Primary & Relay File/Pos:
MASTER_LOG_FILE:DEFAULTis empty, and empty means “first”.MASTER_LOG_POS/RELAY_LOG_POS:DEFAULTis 4- silently set to
DEFAULTif set to less
- silently set to
- ignored with a warning when using GTID
MASTER_PORT:DEFAULTis the MariaDB port- 0 is (technically) invalid.
IGNORE_SERVER_IDS/DO_DOMAIN_IDS/IGNORE_DOMAIN_IDS:DEFAULTis empty.MASTER_DELAY:DEFAULTis 0.int32_ton paper, unsigned in practice: errors if set larger thanMASTER_DELAY_MAX(0x7F'FF'FF'FF)
No DEFAULT – “recognizes” as an error
RELAY_LOG_FILE-
The
CHANGE MASTERstatement usually deletes all relay log files. However, if theRELAY_LOG_FILEand/orRELAY_LOG_POSoptions are specified, then existing relay log files are kept.
⸺ https://mariadb.com/docs/server/reference/sql-statements/administrative-sql-statements/replication-statements/change-master-to#relay-log-options - Empty is invalid.
- GTID preferences aside, what would “first” be useful for?
- also ignored with a warning when using GTID
-
MASTER_HOST&MASTER_USER: “DEFAULT” is empty, which can be set, but the IO thread will err during startup.
| (slave_net_timeout/2.0)); | ||
| if (lex_mi->heartbeat_period) | ||
| lex_mi->heartbeat_period(mi); | ||
| mi->received_heartbeats= 0; // counter lives until master is CHANGEd |
There was a problem hiding this comment.
What was the intent to have CHANGE MASTER always reset this received_heartbeats?
Was it because master_heartbeat_period is always changed regardless of whether it was configured in the CHANGE MASTER?
bnestere
left a comment
bnestere
left a comment
There was a problem hiding this comment.
Hi @ParadoxV5 !
Thanks for the big piece of work, and refactoring along with it! Generally, it looks good, where I left some various comments. A few other additional things not mentioned at specific points:
Additional things:
- It is hard to tell based on the current structure of your commits what you intend the final structure to be, but I'd assume it would be 1 commit for the refactor (MDEV-37530) and another for the extensions (MDEV-28302). I didn't go through your current commits one-by-one because there are too many, but using your PR descriptions as the starting point, some additional things I'd ask to mention/explain upon:
- In the git commit for the refactoring:
2a. Provide some specifics when justifing the refactoring (i.e. what was the old way of doing things, and why it wasn't sufficient)
2b. Explain the refactoring (what are the new class hierarchies, how does it solve the problems in the old code base, and any other benefits it may bring).
2c. Explain how the old code maps to the new code (i.e. it should be able to provide good pointers for how to resolve any merge conflicts this induces). - In the git commit for the feature itself
3a. Rather than using "master_ssl*" to cover those items, list out each item specifically. If we ever add more ssl options in the future, it will be confusing.
| 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 *>(); |
There was a problem hiding this comment.
Oops, I missed that you'd replied. I'd asked Monty about the supplying our own C++ allocators to std functions, to which he'd said
No work ever planned for allocator implementation. The issue is that for most cases MariaDB string functions are superior to std:: versions (performance, support of all MariaDB character sets, usage of mem_root etc). In general I want people to use our functions instead of std:: to not get code bloat and keep a standard way of doing things.
So I'd still say we should use our own Hash_set here.
The persistence code of CHANGE MASTER values needs to match file
lines with the formatting for the corresponding field’s type.
This is unstructured, repetitive (not DRY), and makes feature expansions
(such as MDEV-28302 `CHANGE MASTER …=DEFAULT`)
error-prone if not difficult.
This commit moves these functions and global constants from
`slave.cc`/`.h`, as well as the Master and Relay Log Info File
entries from `Rpl_mi` and `Rpl_rli`, to dedicated `rpl_*info_file.h`
files and under corresponding structs to facilitate organization.
Namely, this commit wraps those file entries with transparent
structs that inherit from a `Persistent` interface,
or shared helper structs that themselves inherit from `Persistent`.
By moving the file read/write helper functions to or behind
implementations of `Persistent`’s virtual methods,
reading or writing the file only takes a type-agnostic loop
over the (wrapped) CHANGE MASTER values.
* This commit also includes preemptive support
for preserving MDEV-28302’s `=DEFAULT`.
As such, unset fields (namely `master_connect_retry`)
now remember their `DEFAULT` states
rather than whatever the default is at CHANGE MASTER time.
* For consistency’s sake, `master_heartbeat_period` is
now reset at RESET REPLICA instead of CHANGE MASTER.
As this refactor will disconnect it from fixes for
some open bugs in prior versions, this commit also:
* Reimplements the value reader functions to be strict with their input
* Fixes MDEV-38010
number parsing ignores trailing garbage and overflows
* Supercedes MDEV-38020 integer overflow
* Changes master_heartbeat_period from a `float` of seconds
to a `uint32_t` of milliseconds (adding `/1000.0`s as needed)
* Fixes MDEV-35879 `Slave_heartbeat_period` is imprecise
* The maximum of `master_heartbeat_period` has been
increased to 4294967.295, i.e., (2³²-1)÷1000.
* `master_heartbeat_period` now rounds
instead of truncates (rounds down).
* Prepares to make `master_retry_count` 64-bit on
LLP64 (e.g., Windows) to match LP64 (e.g., Linux)
Many CHANGE MASTER fields typically have the same configurations between multi-source connections – namely: master_connect_retry master_ssl master_ssl_ca master_ssl_capath master_ssl_cert master_ssl_cipher master_ssl_key master_ssl_verify_server_cert master_ssl_crl master_ssl_crlpath master_use_gtid master_retry_count master_heartbeat_period When MDEV-25674 added `master_retry_count` to CHANGE MASTER, it kept the server option `--master-retry-count` to be its default value. This commit back-adds corresponding server options for the defaults of the rest of those fields. With them, the command line or config files can set up common configurations across replication sources (and even replicas). CHANGE MASTER can override unset (defaulted) configurations per connection. This commit also adds `DEFAULT` keyword support for all of those fields, so overridden configurations can reset to the default without RESET REPLICA. Supporting passing the `DEFAULT` keyword also enables setting `master_connect_retry` and `master_retry_count` to 0, which was previously disregarded. While here, the commit also increases `master_retry_count` to 64-bit on LLP64 (e.g., Windows) to match LP64 (e.g., Linux).
|
I have squashed the commits. (No additional changes or rebasing for this one.) Fwd: @vuvova, for the preview release P.S. The loose iterations are kept on branch MDEV-37530.bak if we still need them. |
andrelkin
left a comment
andrelkin
left a comment
There was a problem hiding this comment.
Looks good to me.
To the design perhaps one day users will ask for extending the new command-line options onto being @@global:s.
I thought about this at the beginning of development: Then users will ask for these |
bnestere
left a comment
bnestere
left a comment
There was a problem hiding this comment.
Oops, I forgot to formally approve. Approved! Thanks, @ParadoxV5
Description
2. MDEV-28302 Configurable defaults for CHANGE MASTER
Many CHANGE MASTER fields typically have the same configurations between multi-source connections – namely:
When MDEV-25674 added
master_retry_countto CHANGE MASTER, it kept the server option--master-retry-countto be its default value.This commit back-adds corresponding server options for the defaults of the rest of those fields.
With them, the command line or config files can set up common configurations across replication sources (and even replicas).
CHANGE MASTER can override unset (defaulted) configurations per connection.
This commit also adds
DEFAULTkeyword support for all of those fields, so overridden configurations can reset to the default without RESET REPLICA.Side Effects
Supporting passing the
DEFAULTkeyword also enables settingmaster_connect_retryandmaster_retry_countto 0, which was previously disregarded.While here, the commit also increases
master_retry_countto 64-bit on LLP64 (e.g., Windows) to match LP64 (e.g., Linux).1. MDEV-37530 Refactor Master & Relay Log info to iterable tuples
The persistence code of CHANGE MASTER values needs to match file lines with the formatting for the corresponding field’s type.
This is unstructured, repetitive (not DRY), and makes feature expansions (such as MDEV-28302
CHANGE MASTER …=DEFAULT) error-prone if not difficult.This commit moves these functions and global constants from
slave.cc/.h, as well as the Master and Relay Log Info File entries fromRpl_miandRpl_rli, to dedicatedrpl_*info_file.hfiles and under corresponding structs to facilitate organization.Namely, this commit wraps those file entries with transparent structs that inherit from a
Persistentinterface, or shared helper structs that themselves inherit fromPersistent.By moving the file read/write helper functions to or behind implementations of
Persistent’s virtual methods, reading or writing the file only takes a type-agnostic loop over the (wrapped) CHANGE MASTER values.This commit also includes preemptive support for preserving MDEV-28302’s
=DEFAULT.As such, unset fields (namely
master_connect_retry) now remember theirDEFAULTstates rather than whatever the default is at CHANGE MASTER time.Side Effects
For consistency’s sake,
master_heartbeat_periodis now reset at RESET REPLICA instead of CHANGE MASTER.(Yes, CHANGE MASTER previously resets it even when there is no
master_heartbeat_period=DEFAULT.)As this refactor will disconnect it from fixes for some open bugs in prior versions, this commit also:
floatof seconds to auint32_tof milliseconds (adding/1000.0s as needed)Slave_heartbeat_periodis imprecisemaster_heartbeat_periodhas been increased to 4294967.295, i.e., (2³²-1)÷1000.master_heartbeat_periodnow rounds instead of truncates (rounds down).master_retry_count64-bit on LLP64 (e.g., Windows) to match LP64 (e.g., Linux)How can this PR be tested?
main.change_master_defaultthat runs its (unique) features, and updatesmain.master_retry_count_basicto reflect the adjusted behaviors across the two parts.rpl.rpl_heartbeatandrpl.rpl_heartbeat_basicto match the adjusted behaviors.PR quality check
orand a refactoring, and the PR is based against themainbranch.This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.