Repository navigation
Conversation
ParadoxV5
left a comment
ParadoxV5
left a comment
There was a problem hiding this comment.
Repl_semi_sync_slave is merely a singleton that does not give enough value as a namespace.
Maybe a main-branch refactor is worth to dissolve it into Master_info…
(Relates to MDEV-40941)
There was a problem hiding this comment.
Hi @ParadoxV5 !
I only have one note. The rest looks good. Thanks!
Also note pl.rpl_semi_sync_fail_over fails on debug builds
| var->value= (char*) &local_semi_sync_enabled; | ||
| if (Master_info *mi= | ||
| get_master_info(&thd->variables.default_master_connection, | ||
| Sql_condition::WARN_LEVEL_NOTE)) |
There was a problem hiding this comment.
I don't think we should do this in GA. I imagine it may break existing user scripts. Maybe we can re-visit it with MDEV-40941. For GA, I think Monty's original suggestion makes the most sense:
Repl_semi_sync_slave.m_slave_enabled should also be changed to a counter to know how many slaves are actually enabled.
Alternatively, we don't need a counter. As far as I see, the only time this is used is for showing status variables. We can probably just iterate through the mi cache and check if any are enabled. It'll need a lock, but so does SHOW SLAVE STATUS.
Maybe @knielsen has a preference. I don't.
There was a problem hiding this comment.
Brandon, yes it sounds reasonable.
Seems best to add per-channel configuration of semisync_slave only in main, not older GA releases.
And yes, seems a counter would not be needed as you explained.
There was a problem hiding this comment.
I don't think we should do this in GA.
Do we leave GA unfixed, then?
I imagine it may break existing user scripts. Maybe we can re-visit it with MDEV-40941.
Reminds me of those malformed Replication packets. We need clear lines: What’s intended, what’s unïntended, and what’s merely undefined behaviour that MDEV-40941 is now defining?
There was a problem hiding this comment.
No we should fix the error and replication break in GA. I mean this particular code change will change the result of rpl_semi_sync_slave_status depending on which connection is configured. My understanding of the pre-patch behavior is if any connections have semi_sync working, the status var will output ON. The behavior proposed here may be ON or OFF depending on default_master_connection. We should maintain the behavior of ON if any channel is actively using semi_sync.
I agree we should define behaviors to enforce (for everything, always), but that should be done in new versions (not GA). I.e., if we've already released a version with some behavior, we shouldn't switch it on a whim in a minor release (unless it is a bug, of course).
So then whenever we'd decide to do MDEV-40941, that would be in a new release, where we could change things if it is well justified.
Does this answer your question?
There was a problem hiding this comment.
I mean this particular code change will change the result of rpl_semi_sync_slave_status depending on which connection is configured. My understanding of the pre-patch behavior is if any connections have semi_sync working, the status var will output
ON. The behavior proposed here may beONorOFFdepending ondefault_master_connection. We should maintain the behavior ofONif any channel is actively using semi_sync.
We can test this, but I’m pretty sure the old behaviour is equivalent to showing what @@rpl_semi_sync_slave_enabled (which my patch does not change, so scripts (that aren’t relying on broken behaviour) shouldn’t break) was last applied for any connection.
I should be able to maintain this, for whatever it’s worth.
We should maintain the behavior of
ONif any channel is actively using semi_sync.
This would make it reminiscent of #5753 (review).
Does this answer your question?
Yes, I suppose.
There was a problem hiding this comment.
@bnestere change applied (current 2nd commit).
I’ve also squashed the original commits into one and rebased onto 11.4.
`@@rpl_semi_sync_slave_enabled` can be changed without stopping replication, because it takes effect when a connection is established. But when it takes effect, it applies to *all* replication connections. This means that starting an async connection will switch any ongoing semi-sync connections to async as well, and vice versa, which will cause those connections to fail to replicate in the wrong mode. This fix resolves this oversight by changing the value application to specific to only the establishing connection (technically, moving the singleton’s field `Repl_semi_sync_slave::m_slave_enabled` to the instance variable `Master_info::semi_sync_enabled`). This designs build toward MDEV-40941, which proposes migrating `@@rpl_semi_sync_slave_enabled` to a proper per-connection config. Note, `Master_info::semi_sync_reply_enabled` must remain separate, because it only disables ACK replying in async slave fallback, whereas the semi-sync slave setting also controls whether the IO thread should expect additional semi-sync flags in event packets. The status variable `Rpl_semi_sync_slave_status` is left referencing ??? ??? Reviewed-by: Brandon Nesterenko <brandon.nesterenko@mariadb.com>
the status variable `Rpl_semi_sync_slave_status` is kept showing the applied `@@rpl_semi_sync_slave_enabled` for the most recent START SLAVE in any connection; that is, not yet made per-connection.
3 participants
@@rpl_semi_sync_slave_enabledcan be changed without stopping replication, because it takes effect when a connection is established.But when it takes effect, it applies to all replication connections.
This means that starting an async connection will switch any ongoing semi-sync connections to async as well, and vice versa, which will cause those connections to fail to replicate in the wrong mode.
This fix resolves this oversight by changing the value application to be specific to only the establishing connection (technically, moving the singleton’s field
Repl_semi_sync_slave::m_slave_enabledto the instance variableMaster_info::semi_sync_enabled).This design builds toward MDEV-40941, which proposes migrating
@@rpl_semi_sync_slave_enabledto a proper per-connection config.Note,
Master_info::semi_sync_reply_enabledmust remain separate, because it only disables ACK replying in async slave fallback (whether that’s the right move is a separate topic), whereas the semi-sync slave setting also controls whether the IO thread should expect additional semi-sync flags in event packets.(It’s also a possible design to ACK in async replication as slave heartbeating.)
To maintain backward compatibility until MDEV-40941, the status variable
Rpl_semi_sync_slave_statusis kept showing the applied@@rpl_semi_sync_slave_enabledfor the most recent START SLAVE in any connection; that is, not yet made per-connection.