Skip to content

MDEV-40122: +DEFAULT is not a valid value for master_heartbeat_period #5491

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.

Sign up for GitHub

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

Open
prathamesh04 wants to merge 1 commit into
base: 12.3
Choose a base branch
from
+18 −4
Open

MDEV-40122: +DEFAULT is not a valid value for master_heartbeat_period #5491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions mysql-test/main/change_master_default.result
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,11 @@ master_ssl_crlpath
using_gtid Slave_Pos
master_retry_count 100000
slave_heartbeat_period 60.000
#
# MDEV-40122: `+DEFAULT` is not a valid value for master_heartbeat_period
#
CHANGE MASTER TO master_heartbeat_period= +DEFAULT;
ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'DEFAULT' at line 1
# Clean-up
DROP PROCEDURE show_defaultable_fields;
RESET SLAVE 'unset' ALL;
Expand Down
9 changes: 9 additions & 0 deletions mysql-test/main/change_master_default.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,15 @@ FROM information_schema.slave_status ORDER BY connection_name;
--query_vertical CALL show_defaultable_fields()


--echo #
--echo # MDEV-40122: `+DEFAULT` is not a valid value for master_heartbeat_period
--echo #

# The `+` prefix may only precede a numeric literal, not `DEFAULT`
--error ER_PARSE_ERROR
CHANGE MASTER TO master_heartbeat_period= +DEFAULT;


--echo # Clean-up

DROP PROCEDURE show_defaultable_fields;
Expand Down
8 changes: 4 additions & 4 deletions sql/sql_yacc.yy
View file
Open in desktop

@ParadoxV5 ParadoxV5 Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Frankly, I’d suggest avoiding breaking num_or_default back to NUM_literal and DEFAULT cases, and instead move the opt_plus under num_or_default’s definition.
But gkodinov and vuvova have already pushed the dissolution of num_or_default.

It’s true that master_heartbeat_period is the only decimal option in CHANGE MASTER, even in the foreseeable future.
But the motivation behind the X_or_defaults (at least the other ones) is to avoid repeating the MASTER_HEARTBEAT_PERIOD_SYM part of the definition, once for the explicit value and once for DEFAULT.
The ideal design was to pass both values and DEFAULTs uniformly to the options themselves… but the path to it turned out to be a lot longer than I could cram when working on MDEV-28302.

Inlining num_or_default’s branches into master_heartbeat_period’s definition also makes a seemingly complex change in contrast to moving the opt_plus.

@prathamesh04 prathamesh04 Aug 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done — opt_plus is now part of num_or_default (num_or_default := opt_plus NUM_literal | DEFAULT), and the action code in master_def stays unified. Thanks for the suggestion.

@vuvova vuvova Aug 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh. Okay. When I first saw the description of this issue — before I looked at the code — I did a fix locally exactly this way, moving opt_plus into num_or_default. But then I saw your first fix and I kind of liked how small and simple the DEFAULT branch became and I thought your fix was better.

Well, anyway, no need to redo it again. Keep it the way @ParadoxV5 wants, it's his code after all.

@ParadoxV5 ParadoxV5 Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It’s already hard to tell apart someone who agreed after consideration and a yes-man, and these days it’s even harder with 96%-AI vibers in the mix, innit?
Offence not intended; I’m just sombre about the downhill trend as an honest associate dev.

In plainer language:
My suggestion was less about “what I want”, but more to prompt thought on whether the simplicity of the separated branches outweighs num_or_default’s consolidation.

@prathamesh04 prathamesh04 Aug 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for being direct about it — that's fair, and the design question deserves a real answer rather than a quick "done".

I did weigh the two forms before settling. The separated branches (opt_plus NUM_literal / DEFAULT inline in master_def, no num_or_default) are more explicit and would sidestep the $$-initialization trap entirely. What kept me on the consolidated form is that master_def already uses the same X_or_default pattern for its other options, so keeping num_or_default makes the heartbeat branch read like the rest of the grammar instead of being the odd one out. Both are defensible; I chose consistency over brevity, and I'm comfortable with that trade-off.

And to be plain about the yes-man part — none taken, and I'd rather earn the approval than have it handed over. The buildbot crash was mine: moving opt_plus into num_or_default left $$ uninitialized, and I should have caught that before CI did. I verified the fix against the full grammar with bison 3.7.5 and 3.8.2 and on the builder that originally crashed, so this version is the result of checking, not of agreeing.

If you have a concrete reason the separated form is better I'll switch to it; otherwise I'd like to keep the consolidated one.

Original file line number Diff line number Diff line change
Expand Up @@ -2423,14 +2423,14 @@ master_def:
{ mi->master_ssl_crlpath= path; };
}

| MASTER_HEARTBEAT_PERIOD_SYM '=' opt_plus num_or_default
Comment thread
prathamesh04 marked this conversation as resolved.
| MASTER_HEARTBEAT_PERIOD_SYM '=' num_or_default
{
if ($4)
if ($3)
{
uint32_t milliseconds;
bool overprecise;
auto decimal_buf= my_decimal(),
*decimal= $4->val_decimal(&decimal_buf);
*decimal= $3->val_decimal(&decimal_buf);
DBUG_ASSERT(decimal);
if (Master_info_file::Heartbeat_period_value::from_decimal(
milliseconds, *decimal, overprecise
Expand Down Expand Up @@ -2490,7 +2490,7 @@ master_use_gtid_enum:
| DEFAULT { $$= enum_master_use_gtid::DEFAULT; }
;
num_or_default:
NUM_literal { DBUG_ASSERT($$); }
opt_plus NUM_literal { DBUG_ASSERT($2); $$= $2; }
| DEFAULT { $$= nullptr; }
;

Expand Down