MDEV-38454 CHANGE MASTER TO master_heartbeat_period does not accept numbers with + sign - #4617
Conversation
bnestere
added
External Contribution
ParadoxV5
left a comment
ParadoxV5
left a comment
There was a problem hiding this comment.
I’m back from Weekend Wednesday!
Thanks for including a test as well!
Although I don’t like how the older tests are named mdev_ABCD – very undescriptive.
Though for this topic, I suggest including the master_heartbeat_period=+60 test under the existing CHANGE MASTER test, which would be mysql-test/suite/rpl/t/rpl_heartbeat_basic.test by convention (if not rpl.rpl_heartbeat and it seems to have have their purposes mixed together).
master_connect_retry=+60 would belong to the corresponding CHANGE MASTER test… if there is one.
It and other fields are not the focus, anyhow.
(This is the status quo… I’d actually prefer gathering ALL format tests together in one script.)
As for the slave_net_timeout warning on amd64-debian-12-debug-embedded – It might be because the test needs --source include/not_embedded.inc, which in the rpl suite is normally practically covered by rpl_init.inc and co..
(We still can’t just get rid of Embedded builds… oh well.)
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for working on this!
This is a preliminary review. Since this is a bug and (IMHO) is a relatively safe to fix, I'd use 10.6 to base my fix on.
5aa4002 to
d07696f
Compare
d07696f to
edb3fed
Compare
Moved the test to |
edb3fed to
2eacad6
Compare
? Is GitHub not updated, or did you accidentally rebased and pushed the pre-move ( |
Ah that was a mistake... I just tried to change the merge target and tweaked my local repo and it accidentally reverted to the previous version.... Will change it back shortly. |
3cd5fb6 to
d560284
Compare
Should be good now. Please check again |
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
All the rest of it is good. Please just use ulong_num. And add tests about it: e.g. with a hex num, with a float etc.
ParadoxV5
left a comment
ParadoxV5
left a comment
There was a problem hiding this comment.
ＬＧＴＭ
Anything more to add, Georgi-san?
| eval CHANGE MASTER TO MASTER_HOST='127.0.0.1', MASTER_PORT=$MASTER_MYPORT, MASTER_USER='root', MASTER_CONNECT_RETRY=$connect_retry, MASTER_HEARTBEAT_PERIOD=+60; | ||
| SHOW GLOBAL STATUS LIKE 'slave_heartbeat_period'; | ||
| RESET SLAVE; | ||
| # Test without + sign (should still work) |
There was a problem hiding this comment.
I was about to say,
- Please use line breaks to keep lines within 80-character wide.
- Alternatively, use the style
--evalwithout;.
No line breaks in this comment-like style, however.
- Alternatively, use the style
- These RESETs could be omitted by testing with different values each time.
But then, I noticed that these are existing styles in the surrounding code.
So nevermind about them for now.
…umbers with `+` sign Fixed parser inconsistency where CHANGE MASTER TO master_heartbeat_period rejected numeric values with an explicit '+' sign, while other parameters like master_connect_retry accepted them. The issue was in sql/sql_yacc.yy where master_heartbeat_period used NUM_literal (which doesn't accept '+'), while other parameters used ulong_num (which includes opt_plus). Solution: Added opt_plus before NUM_literal in the master_heartbeat_period grammar rule, making it consistent with other numeric parameters. Added test case to verify: - master_heartbeat_period=+60 now works (was broken) - master_heartbeat_period=60 still works (backward compatible) All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
4 participants
Description
Fixed parser inconsistency where CHANGE MASTER TO master_heartbeat_period rejected numeric values with an explicit '+' sign, while other parameters like master_connect_retry accepted them.
The issue was in sql/sql_yacc.yy where master_heartbeat_period used NUM_literal (which doesn't accept '+'), while other parameters used ulong_num (which includes opt_plus).
Solution: Added opt_plus before NUM_literal in the master_heartbeat_period grammar rule, making it consistent with other numeric parameters.
How can this PR be tested?
Execute the
mdev_38454.testinrpltest suite in mysql-test-run.Basing the PR against the correct MariaDB version
Backward compatibility
This is a bug fix which allows "+" sign for "master_heartbeat_period". It is compatible with previous versions.
Copyright
All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.