Skip to content

MDEV-39788: Remove added line in master.info format - #5301

Merged
ParadoxV5 merged 1 commit into
12.3from
MDEV-39788
Jul 28, 2026
Merged

MDEV-39788: Remove added line in master.info format#5301
ParadoxV5 merged 1 commit into
12.3from
MDEV-39788

Conversation

@ParadoxV5

@ParadoxV5 ParadoxV5 commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Note

This PR is based on #5147, which provides the regression test.

The line-count lines in master.info and relay-log.info have been inconsistent (off by one) since their introduction.
MDEV-37530 “fixed” this with its common code merger by chance, changing master.info to use relay-log.info’s line-count definition.
This change, howëver, affected backward compatibility, as master.info now expects an ignored MySQL-only line where the first key=value option, master_use_gtid, is.

Since this legacy text-based format has limitations that make it due for replacement, only code reüsablility is valuable, and its consistency does not outweigh its compatibility.
Therefore, this commit solves this problem without reverting code by:

  • Changing the writing code to be compatible with both interpretations
    (albeit inconsistent with the reading code)
  • Adding a shim entry to master.info’s list to emulate prior versions’ reading behaviour
    • Although this solution can only restore upgrade compatibility with versions 10.0+, versions before MariaDB 10 have long been EOL.

While here, this commit also fixes code and comments that contradict the actual effect.

@ParadoxV5
ParadoxV5 requested a review from bnestere June 30, 2026 00:25
@ParadoxV5 ParadoxV5 added MariaDB Corporation Replication Patches involved in replication labels Jun 30, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request addresses compatibility issues in parsing *.info files (specifically master.info and relay-log.info) from older MariaDB/MySQL versions, ensuring that line counts and position values greater than 2^31 are handled correctly. It introduces a LINE_COUNT_FIX pseudo-value in Master_info_file to restore compatibility with files generated by MariaDB 10.0+ and updates the padding and saving logic in Info_file. Additionally, new tests are added to verify these changes. The review feedback suggests naming the unnamed struct LINE_COUNT_FIX to Line_count_fix_t to improve readability and prevent potential compiler-specific issues, along with a minor typo correction in the documentation.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +543 to +559
protected:
/**
@ref Master_info_file has been inconsistent that its line count included
the line-count "value", while @ref Relay_log_info_file's excludes it.
Although the use common code now makes @ref Master_info_file match,
files from older versions are now off-by-one to Info_file::load_from_file().
The presence of this non-reading pseudo-value restores
compatibility with files generated by MariaDB 10.0+,
as the driver loop will increment the line counter for it,
thus emulating counting the line-count "value".
@deprecated This is not needed after all pre-12.3 versions go EoL.
*/
struct: Persistent
{
bool load_from(IO_CACHE *file) override { return false; } ///< No-op
void save_to(IO_CACHE *file) override {} ///< No-op
} LINE_COUNT_FIX;

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.

medium

Naming the unnamed struct Line_count_fix_t improves code readability and avoids potential compiler-specific issues with unnamed classes overriding virtual functions or being used in template-based pointer-to-member wrappers. Additionally, there is a minor typo in the comment: 'use common code' should be 'use of common code'.

protected:
  /**
    @ref Master_info_file has been inconsistent that its line count included
    the line-count "value", while @ref Relay_log_info_file's excludes it.
    Although the use of common code now makes @ref Master_info_file match,
    files from older versions are now off-by-one to Info_file::load_from_file().
    The presence of this non-reading pseudo-value restores
    compatibility with files generated by MariaDB 10.0+,
    as the driver loop will increment the line counter for it,
    thus emulating counting the line-count "value".
    @deprecated This is not needed after all pre-12.3 versions go EoL.
  */
  struct Line_count_fix_t : Persistent
  {
    bool load_from(IO_CACHE *file) override { return false; } ///< No-op
    void save_to(IO_CACHE *file) override {} ///< No-op
  } LINE_COUNT_FIX;

@ParadoxV5 ParadoxV5 changed the title MDEV-39788: Fix master.info not upgrading master_use_gtid MDEV-39788: Remove added line in master.info format Jul 15, 2026
@ParadoxV5

Copy link
Copy Markdown
Contributor Author

Hi @bnestere (when you’re back from vacation),
I have adjusted the code to remove the extra blank line in both the saving and loading code.

@bnestere bnestere left a comment

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.

Hi @ParadoxV5 !

Just to confirm my understanding of your patch: you make the de-facto behavior of the line number exclusive treatment (i.e. matching the relay-log-info file behavior), and then the master_info_file has an extra no-op line to increment the total line count with no other affects, so it reads as inclusive. Is that right?

@ParadoxV5

Copy link
Copy Markdown
Contributor Author

Just to confirm my understanding of your patch: you make the de-facto behavior of the line number exclusive treatment (i.e. matching the relay-log-info file behavior), and then the master_info_file has an extra no-op line to increment the total line count with no other affects, so it reads as inclusive. Is that right?
@bnestere

Well, this description matches the design before my latest push closer than my latest version.

In the latest version, load_from_file() is as described.
save_from_file(), on the other hand, technically uses inclusive treatment (i.e. matching the master.info behavior), but utilizes the fact that the treatment choice does not actually impact how relay-log.info is written.

@bnestere bnestere left a comment

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.

Thanks for the fix @ParadoxV5 !

I've left a few notes.

Comment thread mysql-test/main/rpl_read_new_info.test Outdated
Comment thread mysql-test/std_data/master-10.0.info Outdated
Comment thread mysql-test/main/rpl_read_info.test Outdated
Comment thread sql/rpl_master_info_file.h Outdated
@ParadoxV5

ParadoxV5 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Done @bnestere

P.S. TODO squash when I rebase for merging

@ParadoxV5
ParadoxV5 requested a review from bnestere July 22, 2026 01:18
@ParadoxV5 ParadoxV5 self-assigned this Jul 22, 2026

@bnestere bnestere left a comment

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.

Looks good to me, with a couple notes.

@knielsen may also want to take a look, please check with him before pushing.

Comment thread debian/mariadb-test-data.lintian-overrides Outdated
Comment thread mysql-test/main/rpl_read_info.test Outdated

@knielsen knielsen left a comment

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.

Thanks for this Jimmy. Looks good to me.
I didn't spend a lot of time to understand everything in detail, but looks very reasonable.

@ParadoxV5 ParadoxV5 mentioned this pull request Jul 27, 2026
@ParadoxV5

ParadoxV5 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

waiting for #5147 to reach 12.3 for good record…
(10.11→11.4 and 11.4→11.8 both have merge conflicts currently)

Update:
I’ve been told to just push because the 12.3.3 release cutoff is approaching.

The line-count lines in `master.info` and `relay-log.info`
have been inconsistent (off by one) since their introduction.
MDEV-37530 “fixed” this with its common code merger by chance,
changing `master.info` to use `relay-log.info`’s line-count definition.
This change, howëver, affected backward compatibility,
as `master.info` now expects an ignored MySQL-only line
where the first `key=value` option, `master_use_gtid`, is.

Since this legacy text-based format has limitations that make
it due for replacement, only code reüsablility is valuable,
and its consistency does not outweigh its compatibility.
Therefore, this commit solves this problem without reverting code by:
* Changing the writing code to be compatible with both interpretations
  (albeit inconsistent with the reading code)
* Adding a shim entry to `master.info`’s list
  to emulate prior versions’ reading behaviour
  * Although this solution can only restore upgrade compatibility with
    versions 10.0+, versions before MariaDB 10 have long been EOL.

While here, this commit also fixes code and
comments that contradict the actual effect.

[P.S.] The test for this regression is pushed to 10.11 in PR #5147.

Reviewed-by: Brandon Nesterenko <brandon.nesterenko@mariadb.com>
@ParadoxV5
ParadoxV5 enabled auto-merge (rebase) July 28, 2026 18:37
@ParadoxV5
ParadoxV5 merged commit 8436fe3 into 12.3 Jul 28, 2026
16 of 19 checks passed
@ParadoxV5
ParadoxV5 deleted the MDEV-39788 branch July 28, 2026 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

MariaDB Corporation Replication Patches involved in replication

Development

Successfully merging this pull request may close these issues.

3 participants