Add restore_extra_restart_args for restore

[5antelope]

Signed-off-by: Yang Wu y.wu4515@gmail.com

Description

When we do a RestoreFromBackup mysqld is bring up with "--skip-grant-tables", "--skip-networking". However, depends on the settings in my.cnf, these two might not be enough.

For example, some people might choose to set mysql with super_read_only, which means with current setup, the restore won't go through because we need to call create table in PopulateMetadataTables

This PR adds restore_extra_restart_args parameter, so that ppl can pass in any extra parameters to bring up mysqld (e.g., --super-read-only=OFF)

Related Issue(s)

^

Checklist

• Should this PR be backported?
• Tests were added or are not required
• Documentation was added or is not required

Deployment Notes

[deepthi]

I'm surprised that you are running into this. What version are you at? This should be fixed by #8362
If you have found an additional code path that needs to be fixed, the right way to do it is to fix it so that super privileges are NOT required. We want to get to a point where vitess can run without super privileges.

[5antelope]

@deepthi it's in the restore code path: https://github.com/vitessio/vitess/blob/main/go/vt/mysqlctl/backup.go#L349 where then it uses vt_dba to create db:

vitess/go/vt/mysqlctl/metadata_tables.go

Line 85 in 93c811b

conn, err := mysqld.GetDbaConnection(context.TODO())

It's not about super privileges, we set super_read_only=ON in our my.cnf, which means until it become primary, no one can write to it - i think in the restore flow, we do want to execute create database command? happy to change if there is any other preferred fix

[deepthi]

I think I need to go back and read what the code was before #8107. It is being assumed that CreateMetadataTables is idempotent, but to me if we are restoring from a backup then those tables already exist and there's no need to attempt to create them again.

[5antelope]

Gentle nudge on this PR, thanks! /cc @aquarapid

[GuptaManan100]

Me and @deepthi discussed the use case for this PR and we think that we should always add --super-read-only=OFF like we do for skip-grant-tables. If the user has not set super-read-only to true, then adding that flag does not change anything, but if they have it to true like you do, then we will need to set it to false. So we should always do it and we don't need the extra flag for it.

The code for metadata tables in restore is future-proof in the sense that if there are new tables added to the metadata in a future release, having this code makes it so that you will able to use backups from a previous release without worrying about these metadata tables. This makes the process of upgrades much smoother, so we want to keep that code.

@Sonne5 could you change the PR to add that argument directly instead of adding a new flag?

[aquarapid]

@Sonne5 we've moved this over to #9240 , so I'm going to close this one.