Repository navigation
MDEV-13718 mysqltest crashes upon --sync_slave_with_master without active connection - #5934
Swayam-code wants to merge 3 commits into
Conversation
…tive connection Report the existing no-active-connection error before querying master status. Stop sync_slave_with_master before it selects a slave after a handled error. Cover disconnected commands, connection preservation and successful replication before and after the error. Assisted-by: Codex:GPT-6
gkodinov
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
You have pinpointed the problem space very well. But I am offering some suggestions to make it more inline with the bug fixing process we follow.
| case Q_SYNC_SLAVE_WITH_MASTER: | ||
| { | ||
| do_save_master_pos(); | ||
| if (do_save_master_pos(command)) |
There was a problem hiding this comment.
Why fail if --save-master-pos fails?
Note that --save-master-pos is a comment. Comments are not supposed to have the script fail. In fact, you could execute the .test file in a SQL client and it will run and skip the comments.
There was a problem hiding this comment.
After an expected error 2006, the early return skips the remaining sync step to avoid using stale master_pos. Would you prefer disconnected save/sync commands to report 2006, or silently do nothing like enable_optimizer_trace?
There was a problem hiding this comment.
it's a good question. Probably one for the final review (so you can actually table this for now).
But I'll bite.
In my book --foo is a comment. Comments are not supposed to fail the script execution. It's OK to report a problem executing a comment. As a warning. But stop? It's against what comments are IMHO.
FWIW, mariadb-test can take the command in one of the following two ways:
- as a comment:
--save-master-pos
SELECT 1;
- as a command:
save_master_pos;
SELECT 2;
Right now I believe there's not much functional difference between the two. Except that if you copy and paste the scripts into e.g. mariadb CLI you'd get an error for the command and just a normal ignore for the comment.
It feels like we can extend that logic towards general optionality of the commands themselves.
But alas, it's not done: --connect ... fails and breaks the script in the same way connect ...; does.
And there's already too many existing test lines built upon that assumption to allow changing it easily.
So, as much as I don't find it natural, a comment can and does break the script execution.
But I was hoping that we can reverse that trend with new developments. Hence the ask.
Makes sense?
There was a problem hiding this comment.
Yes, that makes sense. I will leave the warning-versus-error policy for final review as suggested and keep the current error 2006 behavior in this fix.
|
|
||
|
|
||
| int do_save_master_pos() | ||
| int do_save_master_pos(struct st_command *command) |
There was a problem hiding this comment.
Is this the only command that does this?
I believe enable_optimizer_trace does the same too. Please check the rest as well.
There was a problem hiding this comment.
enable_optimizer_trace already checks for a null mysql handle and returns before issuing a query. I found other command paths worth checking, but have not reproduced those yet, so the broader audit is still incomplete.
There was a problem hiding this comment.
This will be the most typical question I'd ask during an internal code review. It's probably easier and faster to find these than to talk about them. But OK, point made and received. I'll defer to the final review again.
Restore the no-argument save_master_pos helper and use curr_command for error handling. Label the connection echoes in the regression output. Error 2006 handling and the sync early return remain unchanged. Assisted-by: Codex:GPT-6
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
LGTM. Additional points made and acknowledged.
One thing I'd like to request before I hand it off to the final reviewer: please squash all of your 3 commits into a single one. We try to have one commit per logical change per developer. FWIW, github allows you to merge with rebase.
3 participants
save_master_posandsync_slave_with_masterdereference a null client handle after disconnecting the current connection. Report error 2006 and stop before selecting a slave.The regression passes in all three binlog formats on Windows (11.4 Debug client, 13.2 Debug server).
main.mysqltesthits the same shell-quoting failure before and after the fix.MDEV-13718