MDEV-35879 Slave_heartbeat_period is imprecise #4441
+74
−50
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`Master_info::heartbeat_period` was previously a `float`, which only has 7 decimal digits of precision on most platforms, which is less than the maximum of 10 its range allows, let alone superfluous user inputs. This commit solves this issue by changing this field to a `DECIMAL(10, 3)`; specifically: * A `uint32_t` of milliseconds internally, * A `my_decimal` of seconds when parsing CHANGE MASTER, * A `double` of seconds when showing to the user, as reads are not affected by external precision issues While here, this commit also corrects the rounding method from “round down” to “nearest millisecond”.
ParadoxV5
commented
Nov 17, 2025
| Value 5.000 | ||
| connection slave; | ||
| change master to master_host='127.0.0.1',master_port=MASTER_PORT, master_user='root', master_heartbeat_period= 0.0009999; | ||
| change master to master_host='127.0.0.1',master_port=MASTER_PORT, master_user='root', master_heartbeat_period= 0.0004999; |
Contributor
Author
There was a problem hiding this comment.
Should I keep the rounding method change out of 10.11?
Contributor
Author
There was a problem hiding this comment.
Range testing of master_heartbeat_period is duplicated between rpl_heartbeat_basic (runs with mix only) and rpl_heartbeat (tests all three binlog formats).
also, forgot a `;` agAin 🫠
ParadoxV5
commented
Nov 19, 2025
Comment on lines
+2272
to
+2278
| Decimal_from_double(double value): my_decimal() | ||
| { | ||
| int unexpected_error __attribute__((unused))= | ||
| double2my_decimal(E_DEC_ERROR, value, this); | ||
| DBUG_ASSERT(!unexpected_error); | ||
| } | ||
| } MAX_PERIOD= SLAVE_MAX_HEARTBEAT_PERIOD/1000.0, THOUSAND= 1000; |
Contributor
Author
There was a problem hiding this comment.
Suggested change
| Decimal_from_double(double value): my_decimal() | |
| { | |
| int unexpected_error __attribute__((unused))= | |
| double2my_decimal(E_DEC_ERROR, value, this); | |
| DBUG_ASSERT(!unexpected_error); | |
| } | |
| } MAX_PERIOD= SLAVE_MAX_HEARTBEAT_PERIOD/1000.0, THOUSAND= 1000; | |
| Decimal_from_ll(longlong value): my_decimal() | |
| { | |
| int unexpected_error __attribute__((unused))= | |
| int2my_decimal(E_DEC_ERROR, value, false, this); | |
| DBUG_ASSERT(!unexpected_error); | |
| } | |
| } MAX_PERIOD= SLAVE_MAX_HEARTBEAT_PERIOD/1000, THOUSAND= 1000; |
Comment on lines
+2290
to
+2291
| decimal_mul(&rounded, &THOUSAND, &decimal_buffer) | | ||
| decimal2ulonglong(&decimal_buffer, &(Lex->mi.heartbeat_period)); |
Contributor
Author
There was a problem hiding this comment.
Suggested change
| decimal_mul(&rounded, &THOUSAND, &decimal_buffer) | | |
| decimal2ulonglong(&decimal_buffer, &(Lex->mi.heartbeat_period)); | |
| /* | |
| (The ideal order would export to a `double` and *then* multiply, | |
| but disappointingly, decimal2double() is implemented | |
| by printing into a string and parsing that char array.) | |
| */ | |
| decimal_mul(&rounded, &THOUSAND, &decimal_buffer) | | |
| decimal2ulonglong(&decimal_buffer, &(Lex->mi.heartbeat_period)); |
ParadoxV5
added a commit
that referenced
this pull request
Nov 19, 2025
MDEV-35879 Slave_heartbeat_period is imprecise
* Increase maximum to `std::numeric_limits<uint32_t>::max() ÷ 1000`,
i.e., `4294967.295` *exactly*
* Excludes its additional tests as they are exclusive to that issue fix.
* But not the `0.00049` case, which was *changed*, not *added*.
* Some changes are refactors and so are exclusive to the `main` branch.
* Delete tests in `rpl.rpl_heartbeat`
that duplicates `rpl.rpl_heartbeat_basic`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Master_info::heartbeat_periodwas previously afloat, which only has 7 decimal digits of precision on most platforms, which is less than the maximum of 10 its range allows, let alone superfluous user inputs.This commit solves this issue by changing this
field to a
DECIMAL(10, 3); specifically:uint32_tof milliseconds internally,my_decimalof seconds when parsing CHANGE MASTER,doubleof seconds when showing to the user, as reads are not affected by external precision issuesWhile here, this commit also corrects the rounding method from “round down” to “nearest millisecond”.
Release Notes
Fix
CHANGE MASTER TO master_heartbeat_periodimprecision for large valuesHow can this PR be tested?
This commit adds another test value and additional queries to
rpl.rpl_heartbeat_basic.PR quality check
This is a new feature or a refactoring, and the PR is based against themainbranch.