MDEV-39343 Restoring from a mysqldump from older version makes mysql_upgrade version test fail#5373
MDEV-39343 Restoring from a mysqldump from older version makes mysql_upgrade version test fail#5373vaintroub wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses MDEV-39343 by ensuring that mysql_upgrade still checks and upgrades system tables even if the mysql_upgrade_info file indicates the installation is already upgraded. This prevents issues when restoring older dumps on an already upgraded server. Feedback is provided on a test helper file, suggesting the use of the native MTR --write_file command instead of spawning an external Perl process to write the version file, which improves portability and is more idiomatic.
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.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses MDEV-39343, where restoring a dump from an older version could cause the mysql_upgrade version test to fail. Instead of skipping the upgrade entirely when the mysql_upgrade_info file matches the current version, mysql_upgrade will now check and upgrade system tables only (setting opt_systables_only = TRUE). A syntax error was identified in the test script load_dump_and_upgrade.inc where the variable DDIR in the --let command is missing the required $ prefix.
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.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request modifies mysql_upgrade to ensure that system tables are still checked and upgraded even if the installation is already marked as upgraded (addressing MDEV-39343, which occurs when restoring a dump from an older version). Instead of skipping the upgrade entirely when already done, it now defaults to checking system tables only, unless --force is specified. Feedback on the changes identifies a test pollution issue in load_dump_and_upgrade.inc, where conditional cleanup of the mysql_upgrade_info file leaves it behind for tests that do not set $with_mysql_upgrade_info. Unconditional cleanup and clearing of the variable are recommended.
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.
| if ($with_mysql_upgrade_info) | ||
| { | ||
| --remove_file $DDIR/mysql_upgrade_info | ||
| } |
There was a problem hiding this comment.
The mysql_upgrade_info file is created by mysql_upgrade during its execution regardless of whether $with_mysql_upgrade_info was set beforehand. By making the cleanup conditional on $with_mysql_upgrade_info, the file is left behind in the datadir when $with_mysql_upgrade_info is not set (such as in the 10.2 upgrade test). This can lead to test pollution and unexpected behavior in subsequent tests.
Additionally, the $with_mysql_upgrade_info variable is not cleared after use, meaning it will persist and affect subsequent tests (such as the 10.4 upgrade test).
It is recommended to unconditionally clean up the mysql_upgrade_info file and clear the $with_mysql_upgrade_info variable at the end of this script.
--let $DDIR= `select @@datadir`
--remove_file $DDIR/mysql_upgrade_info
--let $with_mysql_upgrade_info=
…upgrade version test fail Do not skip the whole upgrade when mysql_upgrade_info is up to date. System tables can still be from an older version, e.g when a dump from an older server was restored. Only checks of user tables are skipped.
There was a problem hiding this comment.
Pull request overview
This PR changes mysql_upgrade behavior so that when mysql_upgrade_info indicates the current version, the tool no longer exits early; instead it proceeds to verify/repair system tables while still skipping user-table checks. This addresses cases where an older mysql schema is restored from a dump even though mysql_upgrade_info looks up to date (MDEV-39343).
Changes:
- Update
mysql_upgradeto treat an up-to-datemysql_upgrade_infoas “system-tables-only” mode rather than “nothing to do”. - Update MTR expected output for repeated
mysql_upgraderuns to reflect the new “system tables only” path. - Add coverage to simulate restoring older system tables while
mysql_upgrade_infois current, ensuringmysql_upgradestill fixes system tables.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| mysql-test/main/mysql_upgrade.test | Updates expectations for rerun behavior and adds an MDEV-39343 regression test that drops a system-table column and verifies mysql_upgrade restores it. |
| mysql-test/main/mysql_upgrade.result | Refreshes expected output to match the new “Checking system tables only…” path and adds result lines for the new regression test. |
| mysql-test/include/load_dump_and_upgrade.inc | Adds an option to pre-create mysql_upgrade_info to simulate “already upgraded” state when upgrading restored dumps. |
| client/mysql_upgrade.c | Implements the behavioral change: when mysql_upgrade_info matches, run system-table upgrade/repair steps instead of exiting early. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* | ||
| Read the mysql_upgrade_info file to check if mysql_upgrade | ||
| already has been run for this installation of MariaDB | ||
| If mysql_upgrade_info file already shows the current version, check | ||
| only the system tables. They can still be from an older version, | ||
| e.g if a dump from an older server was restored (MDEV-39343). | ||
| */ |
| verbose("This installation of MariaDB is already upgraded to %s.\n" | ||
| "Checking system tables only. Use --force to check all tables.", | ||
| upgrade_from_version); | ||
| opt_systables_only= TRUE; |
Do not skip the whole upgrade when mysql_upgrade_info is up to date.
System tables can still be from an older version, e.g when a dump from
an older server was restored. Only checks of user tables are skipped.