Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MDEV-35635: START SLAVE UNTIL allows CHANGE MASTER TO options #3909

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MohanadKh03
Copy link

@MohanadKh03 MohanadKh03 commented Mar 21, 2025

NOTE: This is still a draft PR as I am still writing its .test file

  • The Jira issue number for this PR is: MDEV-______

Description

This commit separates master's options from slave options in sql_yacc.yy by replacing master_file_def with slave_until_file_def which only contain the options needed in the query:

START SLAVE UNTIL

These are the only options valid in this query
MASTER_LOG_FILE_SYM, MASTER_LOG_POS_SYM, RELAY_LOG_FILE_SYM and RELAY_LOG_POS_SYM

This is only a change in the symbols the START SLAVE command did not use any functionalty related to what was included in master_file_def

Release Notes

How can this PR be tested?

This can be tested using start_slave_until.test file which should test valid options queries and invalid ones
Invalid being anything that is not part of this
MASTER_LOG_FILE_SYM, MASTER_LOG_POS_SYM, RELAY_LOG_FILE_SYM and RELAY_LOG_POS_SYM

Basing the PR against the correct MariaDB version

  • This is a new feature or 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.

PR quality check

  • 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.

START SLAVE UNTIL uses the master_file_def options contains options available in CHANGE MASTER command

This commit separates master's options from slave options in ```sql_yacc.yy``` by replacing master_file_def with slave_until_file_def
which only contain the options needed in the START SLAVE command MASTER_LOG_FILE_SYM, MASTER_LOG_POS_SYM, RELAY_LOG_FILE_SYM and RELAY_LOG_POS_SYM
@svoj svoj added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Mar 21, 2025
Copy link
Contributor

@bnestere bnestere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @MohanadKh03 !

Thanks for submitting the patch with a test case already! I've left a few suggestions to the test, and then a couple more suggestions for the git commit message itself:

  1. Please keep each line to 72 characters in length
  2. Git commit messages don't support markdown, so you can remove/simplify the formatting a bit (though note the PR description does support markdown)
  3. Can you re-phrase/extend the problem description to start off with a description that is more user-facing? That is, a typical user won't know what "master_file_def options" are, but you can describe the problem without using that phrase. Then later in the commit message, you can mention it (as it is good to still mention this for developers).

# Clean up after tests.
DROP TABLE t1;
source include/rpl_end.inc;
# ==== End of test ====
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you echo the "End of test" line? And also end the file in a new line.


connection slave;
-- error ER_PARSE_ERROR
start slave until master_log_file='$master_log_file', master_log_pos=$master_log_pos_1, MASTER_USE_GTID=NO, MASTER_DEMOTE_TO_SLAVE=1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To use value substitution (using for variables set via the let mysqltest construct), you need to start the line with the mysqltest command eval. So this should be

Suggested change
start slave until master_log_file='$master_log_file', master_log_pos=$master_log_pos_1, MASTER_USE_GTID=NO, MASTER_DEMOTE_TO_SLAVE=1;
eval start slave until master_log_file='$master_log_file', master_log_pos=$master_log_pos_1, MASTER_USE_GTID=NO, MASTER_DEMOTE_TO_SLAVE=1;

Though to the complete test case, I don't think you actually need to use substitution at all, as you are just testing parsing options. So ultimately I'd say you could do something like

Suggested change
start slave until master_log_file='$master_log_file', master_log_pos=$master_log_pos_1, MASTER_USE_GTID=NO, MASTER_DEMOTE_TO_SLAVE=1;
--error ER_PARSE_ERROR
start slave until master_log_file='master-bin.000001', master_log_pos=4, MASTER_USE_GTID=NO, MASTER_DEMOTE_TO_SLAVE=1;
--error ER_BAD_SLAVE
start slave until master_log_file='master-bin.000001', master_log_pos=4;

and a few other options to test completeness.

# MDEV-35635
# Testing issues around invalid parameters and START SLAVE UNTIL behavior.

# Include necessary configurations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need any pre-amble source for this test, as it is just testing parsing. I think really all that this test needs are the actual start slave lines, no set-up data is needed. Then, for a "passing" test, you can just set the expectation ER_BAD_SLAVE (as no slave is actually configured, but it proves the parsing has gone through)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements.
Development

Successfully merging this pull request may close these issues.

3 participants