server icon indicating copy to clipboard operation
server copied to clipboard

MDEV:8146 - scripts/mysql_system_tables_fix.sql to use alter table add/drop if not exists/if exists

Open gulshanpr opened this issue 1 year ago • 5 comments

  • [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.

gulshanpr avatar Jan 30 '24 08:01 gulshanpr

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 avatar Jan 30 '24 09:01 grooverdan

@grooverdan Do I need to include test for this PR?, I think it is straight-forward.

gulshanpr avatar Jan 30 '24 16:01 gulshanpr

@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.

grooverdan avatar Jan 30 '24 22:01 grooverdan

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.

grooverdan avatar Jan 30 '24 22:01 grooverdan

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Feb 29 '24 10:02 CLAassistant

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.

vuvova avatar Jun 26 '24 11:06 vuvova

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.

LinuxJedi avatar Jul 15 '24 13:07 LinuxJedi