Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for #12557 #12560

Merged
merged 5 commits into from Feb 3, 2023
Merged

Fix for #12557 #12560

merged 5 commits into from Feb 3, 2023

Conversation

angrybrad
Copy link
Member

Works around a MySQL bug https://bugs.mysql.com/bug.php?id=109685

@mrsimonbennett
Copy link

Hey @angrybrad I am not sure if this is the best fix

This has been affecting our customers, (some of which use SnapShooter to backup craft)

By removing the --single-tranaction flag you're changing the way the backup will run and could cause issues that users would not expect. I know it was not ideal the way MySQL did the bug fix, but it was to fix an issue with the --single-transaction backup

We have been advising users to most ideally update there grant permissions

GRANT RELOAD,PROCESS ON *.* TO 'backups'@'%';
FLUSH PRIVILEGES;

I know its not ideal at all!

@afairhurst
Copy link

@mrsimonbennett

I ran into this issue today (unable to backup database). Your solution to update grant permissions worked for me. Thank you!

@mohamedhafez
Copy link

@mrsimonbennett is there any reason we can't just grant the minimum permissions necessary with GRANT SHOW_TABLES ON *.* TO 'backups'@'%';? will it just fail with other errors later on, or is there a possibility it will work with just SHOW_TABLES for some people?

@mrsimonbennett
Copy link

@mrsimonbennett is there any reason we can't just grant the minimum permissions necessary with GRANT SHOW_TABLES ON *.* TO 'backups'@'%';? will it just fail with other errors later on, or is there a possibility it will work with just SHOW_TABLES for some people?

We need to allow the MySQL user to call some commands that are now required, you can read more about it in the MySQL bug fix. But basically they need to be able to flush the database first now as part of a single transaction backup

@darylknight
Copy link

darylknight commented Jan 30, 2023

For users who land here and are using Ploi, this is what worked for us:

mysql -u username -p
GRANT RELOAD,PROCESS ON *.* TO 'database_user'@'127.0.0.1';
FLUSH PRIVILEGES;

And here's my write-up on the issue https://codeknight.co.uk/blog/fixing-backups-on-craft-cms-since-mysql-broke-mysqldump-in-8-0-32

@brandonkelly brandonkelly self-requested a review as a code owner February 3, 2023 01:01
@brandonkelly brandonkelly merged commit 83a5602 into v3 Feb 3, 2023
@brandonkelly brandonkelly deleted the bugfix/12557 branch February 3, 2023 01:01
@angrybrad
Copy link
Member Author

@mrsimonbennett agree it's all gross, but it's a lowest common denominator workaround. The alternative is that we start requiring a server wide privilege for less privileged users to be able to complete a backup in Craft. Even if we wanted to do that, plenty of hosts won't allow it (like AWS RDS), even though it's probably less of an issue if you're hosting on DO and/or using a provisioning service.

Honestly, I'm hoping they revert the change in the next release as it's quite a bad one.

@darylknight
Copy link

Does removing the single-transaction flag mean that if someone saves an entry during the backup, the backup file will be corrupted? Because it might have already backed up one table that relies on another that has been changed?

@erinbit
Copy link
Contributor

erinbit commented Mar 29, 2023

Hi, we are facing this issue now on fortrabbit.com since we upgraded our mysqldump version. From what I see in this changeset, it looks like you are checking the version of the database server, but then running mysqldump locally from php?

On our hosting platform, these versions happen to differ, so the fix is not applied even though it is needed. MySQL server for one app on our platform is running 8.0.31, but the server where app php code runs has mysqldump version 5.7.41. That mysqldump client version has this issue, but since the server version is okay, your fix here is not applied.

You should probably be checking the output of mysqldump --version instead of trusting the server variables.

This issue will pass with time as we upgrade all mysql servers above 8.0.32, but someone else might face this issue in the future as well :)

@jeromecoupe
Copy link
Contributor

Was bitten by this one #13014

@angrybrad
Copy link
Member Author

angrybrad commented Apr 2, 2023

@esbiorn I don’t think detecting the mysqldump version vs. the MySQL version addresses the underlying issue, though. The underlying problem is that the mysqldump version is out of sync with the database version it’s running against. Or at the very least, using an older version of mysqldump to back up a newer version of MySQL can be problematic.

mysqldump, in general, seems to be pretty backward compatible. I’ve never seen a problem running a newer version of mysqldump against an older database (never even had to use the compatible flag).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants