Repository navigation
Conversation
This PR fixes database backups and clones failing with mixed legacy column charsets. It preserves original data bytes while correctly restoring column defaults and ENUM/SET definitions. Mixtures containing native MySQL JSON remain rejected when a binary dump would be unsafe to restore. Fixes https://forum.virtualmin.com/t/db-backup-error/138131
| my ($file) = @_; | ||
| my $temp; | ||
| my $ok = eval { | ||
| require File::Temp; |
There was a problem hiding this comment.
Why use these modules instead of webmin's built-in function for temp files?
| my $ok = eval { | ||
| require File::Temp; | ||
| require File::Basename; | ||
| open(my $in, '<', $file) || die "Cannot read $file: $!\n"; |
There was a problem hiding this comment.
Just return instead of putting all this in an eval
There was a problem hiding this comment.
thanks! Was the original code generated with AI ?
There was a problem hiding this comment.
Yes, however, it isn't AI's fault that I made it approach the problem incorrectly. As you later mentioned, the right approach was to let the database server handle it.
|
So is this actually modifying the MySQL dump instead of just writing it to a file for backing up? That we have to do that to solve this problem seems over-complicated ... I'm surprised MySQL can't handle this case natively. |
Yeah, we are changing the dump by adding charset settings around the table definitions.
I thought there weren't? I'm gonna take a new, closer look into it! |
You were right! I have fixed it and replaced dump rewriting with native options, adding flags only when they aren't already configured. Thanks! |
| $parameters || ""))) { | ||
| # MySQL accepts underscores in long option names as well as hyphens. | ||
| $opt =~ tr/_/-/; | ||
| if ($opt =~ /^--(?:(skip|disable|enable)-)?(hex-blob|set-charset)(?:=(.*))?$/) { |
There was a problem hiding this comment.
What is happening here? It seems odd to be parsing the command line flags for the mysqldump command..
There was a problem hiding this comment.
No, not really, because Webmin's configured dump command can already include these flags.
There was a problem hiding this comment.
Yes but why do we need to parse out these flags? Is this for a case where the admin changes the default backup command, and we need to change the backup behavior?
There was a problem hiding this comment.
I feel like we don't need to support that special case, because it adds a complexity and no admin should be doing this anyway.
There was a problem hiding this comment.
Is this for a case where the admin changes the default backup command, and we need to change the backup behavior?
I mean that Webmin's configured mysqldump command may already include these flags. We check whether --hex-blob and --skip-set-charset are enabled, so they aren't added twice.
I feel like we don't need to support that special case, because it adds a complexity and no admin should be doing this anyway.
We don't know what an admin can do, and we have no control over that. So the extra logic handles cases like --hex-blob=0 or a later flags overriding an earlier one, and simply checking whether the option name appears wouldn't catch those cases.
There was a problem hiding this comment.
I mean that Webmin's configured mysqldump command may already include these flags.
None of our default configs include those flags though, so this would have to be something the admin goes out of their way to configure.
Can we not just add an extra flags we need at the end of the command? Also, I would question if we really want to override the admin's customizations?
Hello Jamie,
This PR fixes database backups and clones failing with mixed legacy column charsets.
It preserves original data bytes while correctly restoring column defaults and ENUM/SET definitions.
The mixtures containing native MySQL JSON remain rejected when a binary dump would be unsafe to restore.
In response to: https://forum.virtualmin.com/t/db-backup-error/138131