Repository navigation
Conversation
The `Ack_receiver::remove_slave()` call in `kill_zombie_dump_threads()`’s loop callback is not necessary, because binlog dump threads that have called `Ack_receiver::add_slave()` will call `remove_slave()` during cleanup anyway. This commit erases this extraneous call, for it wastes the dump thread’s normal `remove_slave()` step, which locks `Ack_receiver`’s global mutex. MDEV-21322 (report ACKs on SHOW SLAVE HOSTS) also has trouble matching its synchronization needs with this global lock acquisition.
(Draft: Commit message is staged on the PR.)
7115ec7 to
0ec19ff
Compare
There was a problem hiding this comment.
I should mask out the legacy positions, or even the entire columns for these “innocent” tests, right?
We can repurpose the currently unused (!) script include/show_slave_hosts.inc.
There was a problem hiding this comment.
While GTID-based ACKs are trivial to collect into a GTID position, legacy file positions should no longer be ambiguously converted to GTID positions.
I currently present them with the same columns as GTID, in file:pos format.
Is this acceptable?
Or do we prefer separate File-Pos and GTID columns?
Alternatively, we can limit this feature to binlog implementations that support GTID ACKs only, as if legacy file positions are deprecated.
But we’d have to sacrifice legacy binlog availability until MDEV-40331 «Use GTID instead of filename/pos for semisync ack also for legacy binlog», which would hurt adoption.
This execution would make many of my other questions in this batch obsolete, though.
| } | ||
|
|
||
| if (info->thd->slave_info && | ||
| (/*TODO MDEV-40331 */ !opt_binlog_engine_hton || end_of_group_event)) |
There was a problem hiding this comment.
I currently update the legacy position after every event (an atomic number shouldn’t be expensive).
This is reflected in the code name, “Dump_pos” (TODO: I forgot to rename that to sent_pos).
Or should I update only after whole event groups to match GTIDs?
(Note to myself: If choosing this design, then the local Repl_semi_sync_trx_info variable in Repl_semi_sync_master::update_sync_header() can be replaced by this dump position.)
A consideration:
Regardless, this master-side statistic cannot conclude whether a semi-sync slave is up-to-date, because event groups that don’t require ACKs can always push the sent position well beyond the ACKed position.
| if (Slave_info *si= thd->slave_info) | ||
| { | ||
| Repl_pos *&dump_pos= slave_info_dump_pos(si); | ||
| if (opt_binlog_engine_hton) |
There was a problem hiding this comment.
I almost used info->using_gtid_state here, but figured it wouldn’t look right without MDEV-40331: a legacy ACK is incomparable to a GTID dump position.
Though, unless MDEV-40331 is cancelled, I still see the probability of going back to info->using_gtid_state.
The challenge is that info->using_gtid_state is not known when COM_REGISTER_SLAVE constructs the Slave_info, only during COM_BINLOG_DUMP (which is shared by non-slave clients, namely mariadb-dump).
I could’ve otherwise merged Slave_info with the legacy/GTID sets; until then, this implementation coïncidentally defines “Sent_pos = NULL” to be “async initializing” (including “COM_REGISTER_SLAVE sent but no COM_BINLOG_DUMP yet”).
| auto *slave= new Trx_info_slave<Trans_gtid_pos_info>(); | ||
| if (!slave) | ||
| return true; | ||
| if (slave->trx_info_.Gtid_pos::set(*gtid_state)) |
There was a problem hiding this comment.
I’ve had a back-and-forth with this detail:
Should the ACK position start with (GTID: and include) the initial connection position?
While the initial connection is not technically an ACK (not counted in Rpl_semi_sync_master_get_ack), it’s still considered an ACK in practice since it also reports a synchronized position.
Leaving the ACK position blank at the start will intuitively differentiate the “semi-sync catching up via async” state from the general “semi-sync not up-to-date” status; no additional enum column needed.
But domains not in the ACK position would look like they’re waiting for position 1 even if the slave connected at non-zero positions, just because those domains haven’t received real ACKs.
The tentative choice is “do fill”, because the Trans_binlog_info does double duty as both the ACK position and the transaction identifier in Repl_semi_sync_master_file_pos.
In contrast, “GTID position” is distinct from “GTID as a T.I.D.”.
| // Semi-sync ACK positions are without the directory part | ||
| &(linfo.log_file_name[dirname_length(linfo.log_file_name)]), |
There was a problem hiding this comment.
Why does the other code use LOG_INFO::log_file_name directly (./mysql-bin.000001), but use binlog_send_info with dirlen (mysql-bin.000001)??
This might cause a bug that’s been elusive for decades… 😟
| rpl_gtid gtid; | ||
| const char *log_file; | ||
| my_off_t log_pos; |
There was a problem hiding this comment.
Why does Repl_semi_sync_trx_info have both File-Position and GTID when ACKs are either one or the other?
Yes, Trans_binlog_info needs both to future-proof for mixed identifiers post MDEV-40331, but the abstract Repl_semi_sync_trx_info does not require Trans_binlog_info’s “cramming two position types into one namespace” detail.
| tranx_entry= m_active_tranxs->is_thd_waiter(&inf); | ||
| if (tranx_entry) | ||
| res= | ||
| inf.set(tranx_entry->log_name, tranx_entry->log_pos, gtid); |
There was a problem hiding this comment.
Could the existing code directly use a Trans_binlog_info from the Tranx_node?
| auto *gtid_dump_pos= new Gtid_pos(); | ||
| if (gtid_dump_pos) | ||
| { | ||
| if (gtid_dump_pos->set(info->gtid_state)) |
There was a problem hiding this comment.
Reüsing info->gtid_state would be leaner, since it’s set to the initial position and never incremented.
But the dump thread deletes domains from this position when reaching their start positions!
The comment says it avoids harmlessly checking obsolete domains again.
Though, are start positions of begun domains really obsolete?
In non-strict mode, wouldn’t this optimization become a bug for GTIDs located after but sequenced before the initial sequence numbers?
And what’s the expectation on repeated sequence numbers?
Maybe neither this removal nor SHOW SLAVE HOSTS should modify info->gtid_state…
|
|
||
| rpl_semi_sync_master_get_ack++; | ||
| report_reply_binlog_file_pos(server_id, log_file_name, log_file_pos); | ||
| report_reply_binlog_file_pos(slave, log_file_name, log_file_pos); |
There was a problem hiding this comment.
BTW, why was Ack_receiver::m_mutex (really Ack_receiver::m_slaves’s mutex) still locked going into report_reply_binlog*() when the read ACK is already parsed (e.g., strncpy() above)?
But hey, it’s convenient for me; I can pass the entire Slave!
Then how about TODO for the existing code: Pass a pointer to the packet directly to report_reply_binlog_file_pos() to avoid double-copying?
This feature extends SHOW SLAVE HOSTS with always-available columns
Sent_posandAck_posthat shed light on performance from a master server’s perspective.They are the
file:poss of the end of transactions when using the legacy binlog, and complete GTID positions when using an engine binlog.Sent_posis the position that has just been sent to the slave (in both async and semi-sync).Ack_posis the position that the slave has acknowledged.NULLfor async slaves (or semi-sync slaves that’ve just registered and not yet replicating).Rpl_semi_sync_master_get_ack.Acknowledgements to developers on prior versions of this feature:
@JackSlateur, MDEV-21322: report slave progress to the master #1427@an3l, MDEV-21322: report slave progress to the master #2374@bnestere, MDEV-21322: Report slave progress to the master #3288Significant differences from prior implementations
LOG_INFO::log_file_namebuffer instead.file:4when switching to the next legacy binlog file.Repl_semi_sync_mastermanage “Ack Position”, including protecting this field with its global mutex instead, so the collection of “Ack Position” won’t need to additionally lockTHD::LOCK_thd_data.Slavestruct.Otherwise, keep using File-Position and don’t convert to GTIDs because that’ll be ambiguous with actual GTID ACKs.
NULL.What about “async initializing”?
Rpl_semi_sync_master_statusalready reflects.Frankly, if I frown at my own product, it’s a sign that outside factors are due.
Slave_info) is private torepl_failsafe.cc(which has nothing about a failsafe) and is very inconvenient to access.Slave_info.@@gtid_X_pos.