server
server copied to clipboard
MDEV:8146 - scripts/mysql_system_tables_fix.sql to use alter table add/drop if not exists/if exists
- [x] The Jira issue number for this PR is: MDEV-8146
Description
Added additional checks in scripts/mysql_system_tables_fix.sql to reduce error in log file, update includes IF EXISTS/IF NOT EXISTS for DROP/ADD statements
How can this PR be tested?
This PR updated ADD COLUMN to ADD COLUMN IF NOT EXISTS and same for DROP.
Basing the PR against the correct MariaDB version
- [x] This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced.
- [ ] This is a new feature and the PR is based against the latest MariaDB development branch.
PR quality check
- [x] I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
- [x] For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.
Thanks for the PR - can you rebase this on to the latest 11.4 ((assuming origin is MariaDB/server repo -> git fetch origin; git rebase --onto origin/11.4 HEAD^) and resolve the mysql_system_tables_fix.sql conflict and remove the MDEV-32611 commit. Force push to the same branch when complete.
@grooverdan Do I need to include test for this PR?, I think it is straight-forward.
@grooverdan Do I need to include test for this PR?, I think it is straight-forward.
The existing code is executed in tests. No need to add anything.
Check the error output in any of the failing tests above, there's a few syntax errors. Hopefully should be obvious from the error. I saw at least one that had the IF NOT EXITS before KEY.
My concern is that this is a very fragile script that grew over time to handle all possible combinations of upgrades. From old MariaDB versions, from old MySQL versions, etc. Consider, e.g. MDEV-26363, where MySQL 5.7 -> MariaDB 10.3 upgrade worked fine, but an upgrade further to MariaDB 10.4 failed.
Adding IF [NOT] EXISTS changes individual ALTER TABLE commands from failing if any of the sub-commands is impossible, to executing as many of them as possible. This changes the behavior of the whole script in subtle and non-obvious ways, for example, I definitely cannot predict whether it'll break anything, and what exactly.
I've had discussions with Sergei over this one. Whilst we agree that we would love to be able to merge this, unfortunately we are going to have to reject it at this at this stage. The problem, as Sergei indicated, is that there are many permutations to testing this, and we do not run them all right now. It is possible that a bug might slip though which could break things such as a multi-stage upgrade path.
We would like to thank you for this contribution though, and hopefully we can revisit it when can test for every possible thing that could go wrong.