go/vt/mysqlctl: run backup init sql after catchup, disable super readonly (#19713)

Author: maxenglander

go/cmd/vtbackup/cli/vtbackup.go

@@ -217,8 +217,8 @@ func init() {
 	utils.SetFlagBoolVar(Main.Flags(), &allowFirstBackup, "allow-first-backup", allowFirstBackup, "Allow this job to take the first backup of an existing shard.")
 	utils.SetFlagBoolVar(Main.Flags(), &restartBeforeBackup, "restart-before-backup", restartBeforeBackup, "Perform a mysqld clean/full restart after applying binlogs, but before taking the backup. Only makes sense to work around xtrabackup bugs.")
 	Main.Flags().BoolVar(&upgradeSafe, "upgrade-safe", upgradeSafe, "Whether to use innodb_fast_shutdown=0 for the backup so it is safe to use for MySQL upgrades.")
-	Main.Flags().StringSliceVar(&initSQLQueries, "init-backup-sql-queries", nil, "Queries to execute before initializing the backup")
-	Main.Flags().Var((*topoproto.TabletTypeListFlag)(&initSQLTabletTypes), "init-backup-tablet-types", "Tablet types used for the backup where the init SQL queries (--init-backup-sql-queries) will be executed before initializing the backup")
+	Main.Flags().StringSliceVar(&initSQLQueries, "init-backup-sql-queries", nil, "Queries to execute after catch-up replication, before initializing the backup")
+	Main.Flags().Var((*topoproto.TabletTypeListFlag)(&initSQLTabletTypes), "init-backup-tablet-types", "Tablet types used for the backup where the init SQL queries (--init-backup-sql-queries) will be executed after catch-up replication, before initializing the backup")
 	Main.Flags().DurationVar(&initSQLTimeout, "init-backup-sql-timeout", initSQLTimeout, "At what point should we time out the init SQL query (--init-backup-sql-queries) work and either fail the backup job (--init-backup-sql-fail-on-error) or continue on with the backup")
 	Main.Flags().BoolVar(&initSQLFailOnError, "init-backup-sql-fail-on-error", false, "Whether or not to fail the backup if the init SQL queries (--init-backup-sql-queries) fail, which includes if they fail to complete before the specified timeout (--init-backup-sql-timeout)")
 
@@ -517,11 +517,6 @@ func takeBackup(ctx, backgroundCtx context.Context, topoServer *topo.Server, bac
 		}
 	}
 
-	// Perform any requested pre backup initialization queries.
-	if err := mysqlctl.ExecuteBackupInitSQL(ctx, &backupParams); err != nil {
-		return vterrors.Wrap(err, "failed to execute backup init SQL queries")
-	}
-
 	// We have restored a backup. Now start replication.
 	if err := resetReplication(ctx, restorePos, mysqld); err != nil {
 		return fmt.Errorf("error resetting replication: %v", err)
@@ -559,11 +554,6 @@ func takeBackup(ctx, backgroundCtx context.Context, topoServer *topo.Server, bac
 
 	log.Info("takeBackup: primary position is: " + primaryPos.String())
 
-	// Remember the time when we fetched the primary position, not when we caught
-	// up to it, so the timestamp on our backup is honest (assuming we make it
-	// to the goal position).
-	backupParams.BackupTime = time.Now()
-
 	// Wait for replication to catch up.
 	phase.Set(phaseNameCatchupReplication, int64(1))
 	defer phase.Set(phaseNameCatchupReplication, int64(0))
@@ -641,13 +631,23 @@ func takeBackup(ctx, backgroundCtx context.Context, topoServer *topo.Server, bac
 	phaseStatus.Set([]string{phaseNameCatchupReplication, phaseStatusCatchupReplicationStalled}, 0)
 	phaseStatus.Set([]string{phaseNameCatchupReplication, phaseStatusCatchupReplicationStopped}, 0)
 
-	// Re-enable redo logging.
+	// Re-enable redo logging before running init SQL, so that init SQL
+	// queries run with full crash safety.
 	if disabledRedoLog {
 		if err := mysqld.EnableRedoLog(ctx); err != nil {
 			return fmt.Errorf("failed to re-enable redo log: %v", err)
 		}
 	}
 
+	// Perform any requested backup initialization queries after catch-up replication.
+	if err := mysqlctl.ExecuteBackupInitSQL(ctx, &backupParams); err != nil {
+		return vterrors.Wrap(err, "failed to execute backup init SQL queries")
+	}
+
+	// Set BackupTime after catch-up and init SQL, so the timestamp on the
+	// backup postdates any changes made by init SQL queries.
+	backupParams.BackupTime = time.Now()
+
 	if restartBeforeBackup {
 		restartAt := time.Now()
 		log.Info("Proceeding with clean MySQL shutdown and startup to flush all buffers.")

go/cmd/vtctldclient/command/backups.go

@@ -362,8 +362,8 @@ func init() {
 }
 
 func addInitSQLFlags(cmd *cobra.Command) {
-	cmd.Flags().StringSliceVar(&backupOptions.InitSQLQueries, "init-backup-sql-queries", nil, "Queries to execute before initializing the backup")
-	cmd.Flags().Var((*topoproto.TabletTypeListFlag)(&backupOptions.InitSQLTabletTypes), "init-backup-tablet-types", "Tablet types used for the backup where the init SQL queries (--init-backup-sql-queries) will be executed before initializing the backup")
+	cmd.Flags().StringSliceVar(&backupOptions.InitSQLQueries, "init-backup-sql-queries", nil, "Queries to execute before taking the backup")
+	cmd.Flags().Var((*topoproto.TabletTypeListFlag)(&backupOptions.InitSQLTabletTypes), "init-backup-tablet-types", "Tablet types used for the backup where the init SQL queries (--init-backup-sql-queries) will be executed before taking the backup")
 	cmd.Flags().DurationVar(&backupOptions.InitSQLTimeout, "init-backup-sql-timeout", backupOptions.InitSQLTimeout, "At what point should we time out the init SQL query (--init-backup-sql-queries) work and either fail the backup job (--init-backup-sql-fail-on-error) or continue on with the backup")
 	cmd.Flags().BoolVar(&backupOptions.InitSQLFailOnError, "init-backup-sql-fail-on-error", false, "Whether or not to fail the backup if the init SQL queries (--init-backup-sql-queries) fail, which includes if they fail to complete before the specified timeout (--init-backup-sql-timeout)")
 }

go/flags/endtoend/vtbackup.txt

@@ -153,9 +153,9 @@ Flags:
   -h, --help                                                        help for vtbackup
       --incremental-from-pos string                                 Position, or name of backup from which to create an incremental backup. Default: empty. If given, then this backup becomes an incremental backup from given position or given backup. If value is 'auto', this backup will be taken from the last successful backup position.
       --init-backup-sql-fail-on-error                               Whether or not to fail the backup if the init SQL queries (--init-backup-sql-queries) fail, which includes if they fail to complete before the specified timeout (--init-backup-sql-timeout)
-      --init-backup-sql-queries strings                             Queries to execute before initializing the backup
+      --init-backup-sql-queries strings                             Queries to execute after catch-up replication, before initializing the backup
       --init-backup-sql-timeout duration                            At what point should we time out the init SQL query (--init-backup-sql-queries) work and either fail the backup job (--init-backup-sql-fail-on-error) or continue on with the backup
-      --init-backup-tablet-types strings                            Tablet types used for the backup where the init SQL queries (--init-backup-sql-queries) will be executed before initializing the backup
+      --init-backup-tablet-types strings                            Tablet types used for the backup where the init SQL queries (--init-backup-sql-queries) will be executed after catch-up replication, before initializing the backup
       --init-db-name-override string                                (init parameter) override the name of the db used by vttablet
       --init-db-sql-file string                                     path to .sql file to run after mysql_install_db
       --init-keyspace string                                        (init parameter) keyspace to use for this tablet

go/vt/mysqlctl/backup.go

@@ -29,6 +29,7 @@ import (
 
 	"github.com/spf13/pflag"
 
+	"vitess.io/vitess/go/mysql/sqlerror"
 	"vitess.io/vitess/go/protoutil"
 	"vitess.io/vitess/go/textutil"
 	"vitess.io/vitess/go/vt/log"
@@ -551,6 +552,25 @@ func ExecuteBackupInitSQL(ctx context.Context, params *BackupParams) error {
 	params.Logger.Infof("Executing init SQL queries %q, with a timeout of %v and fail backup on error set to %t", queriesCSV, initTimeout, params.InitSQL.FailOnError)
 	initCtx, cancel := context.WithTimeout(ctx, initTimeout)
 	defer cancel()
+	// Disable super_read_only before executing init SQL queries, because MySQL
+	// may have been started with super-read-only enabled via my.cnf.
+	resetFunc, err := params.Mysqld.SetSuperReadOnly(initCtx, false)
+	if err != nil {
+		if sqlErr, ok := err.(*sqlerror.SQLError); ok && sqlErr.Number() == sqlerror.ERUnknownSystemVariable {
+			params.Logger.Infof("Server does not support super_read_only, continuing with init SQL queries: %v", err)
+		} else if params.InitSQL.FailOnError {
+			return vterrors.Wrap(err, "failed to disable super_read_only for init SQL queries")
+		} else {
+			params.Logger.Infof("Failed to disable super_read_only for init SQL queries: %v. Will still attempt init SQL queries as fail-on-error is false", err)
+		}
+	}
+	if resetFunc != nil {
+		defer func() {
+			if err := resetFunc(); err != nil {
+				params.Logger.Errorf("Failed to reset super_read_only after init SQL queries: %v", err)
+			}
+		}()
+	}
 	if err := params.Mysqld.ExecuteSuperQueryList(initCtx, params.InitSQL.Queries); err != nil {
 		if params.InitSQL.FailOnError {
 			return vterrors.Wrapf(err, "failed to execute init SQL queries %q and instructed to fail backup in this case", queriesCSV)

go/vt/mysqlctl/backup_test.go

@@ -20,6 +20,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -37,6 +38,7 @@ import (
 	"vitess.io/vitess/go/mysql"
 	"vitess.io/vitess/go/mysql/fakesqldb"
 	"vitess.io/vitess/go/mysql/replication"
+	"vitess.io/vitess/go/mysql/sqlerror"
 	"vitess.io/vitess/go/protoutil"
 	"vitess.io/vitess/go/sqltypes"
 	"vitess.io/vitess/go/test/utils"
@@ -928,8 +930,121 @@ func TestExecuteBackupInitSQL(t *testing.T) {
 			wantErrString: "no timeout provided",
 			wantNoLogMsg:  true,
 		},
+		{
+			name: "SetSuperReadOnly failure with FailOnError true",
+			params: &BackupParams{
+				TabletType: topodatapb.TabletType_PRIMARY,
+				InitSQL: &tabletmanagerdatapb.BackupRequest_InitSQL{
+					Queries:     []string{"OPTIMIZE TABLE foo"},
+					TabletTypes: []topodatapb.TabletType{topodatapb.TabletType_PRIMARY},
+					Timeout:     protoutil.DurationToProto(30 * time.Second),
+					FailOnError: true,
+				},
+				Logger: logutil.NewMemoryLogger(),
+			},
+			setupMysqld: func(fmd *FakeMysqlDaemon) {
+				fmd.SetSuperReadOnlyError = errors.New("access denied")
+			},
+			wantErr:       true,
+			wantErrString: "failed to disable super_read_only for init SQL queries",
+		},
+		{
+			name: "SetSuperReadOnly ERUnknownSystemVariable with FailOnError true",
+			params: &BackupParams{
+				TabletType: topodatapb.TabletType_PRIMARY,
+				InitSQL: &tabletmanagerdatapb.BackupRequest_InitSQL{
+					Queries:     []string{"OPTIMIZE TABLE foo"},
+					TabletTypes: []topodatapb.TabletType{topodatapb.TabletType_PRIMARY},
+					Timeout:     protoutil.DurationToProto(30 * time.Second),
+					FailOnError: true,
+				},
+				Logger: logutil.NewMemoryLogger(),
+			},
+			setupMysqld: func(fmd *FakeMysqlDaemon) {
+				fmd.SetSuperReadOnlyError = sqlerror.NewSQLError(sqlerror.ERUnknownSystemVariable, "", "Unknown system variable 'super_read_only'")
+				fmd.ExpectedExecuteSuperQueryList = []string{"OPTIMIZE TABLE foo"}
+			},
+			wantErr:    false,
+			wantLogMsg: "Server does not support super_read_only",
+		},
+		{
+			name: "SetSuperReadOnly failure with FailOnError false",
+			params: &BackupParams{
+				TabletType: topodatapb.TabletType_PRIMARY,
+				InitSQL: &tabletmanagerdatapb.BackupRequest_InitSQL{
+					Queries:     []string{"OPTIMIZE TABLE foo"},
+					TabletTypes: []topodatapb.TabletType{topodatapb.TabletType_PRIMARY},
+					Timeout:     protoutil.DurationToProto(30 * time.Second),
+					FailOnError: false,
+				},
+				Logger: logutil.NewMemoryLogger(),
+			},
+			setupMysqld: func(fmd *FakeMysqlDaemon) {
+				fmd.SetSuperReadOnlyError = errors.New("access denied")
+				fmd.ExpectedExecuteSuperQueryList = []string{"OPTIMIZE TABLE foo"}
+			},
+			wantErr:    false,
+			wantLogMsg: "Failed to disable super_read_only for init SQL queries",
+		},
 	}
 
+	// Test that super_read_only is disabled before query execution and reset after.
+	t.Run("super_read_only disabled and reset after queries", func(t *testing.T) {
+		sqldb := fakesqldb.New(t)
+		defer sqldb.Close()
+		mysqld := NewFakeMysqlDaemon(sqldb)
+		defer mysqld.Close()
+
+		mysqld.SuperReadOnly.Store(true)
+		mysqld.ExpectedExecuteSuperQueryList = []string{"OPTIMIZE TABLE foo"}
+		superReadOnlyDuringQueries := true
+		mysqld.ExecuteSuperQueryListCallback = func() {
+			superReadOnlyDuringQueries = mysqld.SuperReadOnly.Load()
+		}
+
+		params := &BackupParams{
+			TabletType: topodatapb.TabletType_PRIMARY,
+			InitSQL: &tabletmanagerdatapb.BackupRequest_InitSQL{
+				Queries:     []string{"OPTIMIZE TABLE foo"},
+				TabletTypes: []topodatapb.TabletType{topodatapb.TabletType_PRIMARY},
+				Timeout:     protoutil.DurationToProto(30 * time.Second),
+			},
+			Mysqld: mysqld,
+			Logger: logutil.NewMemoryLogger(),
+		}
+
+		err := ExecuteBackupInitSQL(context.Background(), params)
+		require.NoError(t, err)
+		assert.False(t, superReadOnlyDuringQueries, "super_read_only should be disabled during query execution")
+		assert.True(t, mysqld.SuperReadOnly.Load(), "super_read_only should be reset to true after queries complete")
+	})
+
+	// Test that super_read_only is not reset when it was already off.
+	t.Run("super_read_only already off no reset needed", func(t *testing.T) {
+		sqldb := fakesqldb.New(t)
+		defer sqldb.Close()
+		mysqld := NewFakeMysqlDaemon(sqldb)
+		defer mysqld.Close()
+
+		mysqld.SuperReadOnly.Store(false)
+		mysqld.ExpectedExecuteSuperQueryList = []string{"OPTIMIZE TABLE foo"}
+
+		params := &BackupParams{
+			TabletType: topodatapb.TabletType_PRIMARY,
+			InitSQL: &tabletmanagerdatapb.BackupRequest_InitSQL{
+				Queries:     []string{"OPTIMIZE TABLE foo"},
+				TabletTypes: []topodatapb.TabletType{topodatapb.TabletType_PRIMARY},
+				Timeout:     protoutil.DurationToProto(30 * time.Second),
+			},
+			Mysqld: mysqld,
+			Logger: logutil.NewMemoryLogger(),
+		}
+
+		err := ExecuteBackupInitSQL(context.Background(), params)
+		require.NoError(t, err)
+		assert.False(t, mysqld.SuperReadOnly.Load(), "super_read_only should remain false")
+	})
+
 	// Test case for context cancellation during query execution.
 	t.Run("parent context canceled with query failure", func(t *testing.T) {
 		sqldb := fakesqldb.New(t)

go/vt/mysqlctl/fakemysqldaemon.go

@@ -125,6 +125,13 @@ type FakeMysqlDaemon struct {
 	// SuperReadOnly is the current value of the flag.
 	SuperReadOnly atomic.Bool
 
+	// SetSuperReadOnlyError is used by SetSuperReadOnly.
+	SetSuperReadOnlyError error
+
+	// ExecuteSuperQueryListCallback is called at the start of ExecuteSuperQueryList
+	// before any queries are executed, if set.
+	ExecuteSuperQueryListCallback func()
+
 	// SetReplicationPositionPos is matched against the input of
 	// SetReplicationPosition. If it doesn't match, SetReplicationPosition
 	// will return an error.
@@ -456,9 +463,25 @@ func (fmd *FakeMysqlDaemon) SetReadOnly(ctx context.Context, on bool) error {
 
 // SetSuperReadOnly is part of the MysqlDaemon interface.
 func (fmd *FakeMysqlDaemon) SetSuperReadOnly(ctx context.Context, on bool) (ResetSuperReadOnlyFunc, error) {
+	if fmd.SetSuperReadOnlyError != nil {
+		return nil, fmd.SetSuperReadOnlyError
+	}
+	prev := fmd.SuperReadOnly.Load()
+	prevReadOnly := fmd.ReadOnly
 	fmd.SuperReadOnly.Store(on)
-	fmd.ReadOnly = on
-	return nil, nil
+	// In real MySQL, enabling super_read_only implies read_only = ON,
+	// but disabling super_read_only does not change read_only.
+	if on {
+		fmd.ReadOnly = true
+	}
+	if prev == on {
+		return nil, nil
+	}
+	return func() error {
+		fmd.SuperReadOnly.Store(prev)
+		fmd.ReadOnly = prevReadOnly
+		return nil
+	}, nil
 }
 
 // GetGlobalStatusVars is part of the MysqlDaemon interface.
@@ -593,6 +616,9 @@ func (fmd *FakeMysqlDaemon) ExecuteSuperQuery(ctx context.Context, query string)
 
 // ExecuteSuperQueryList is part of the MysqlDaemon interface
 func (fmd *FakeMysqlDaemon) ExecuteSuperQueryList(ctx context.Context, queryList []string) error {
+	if fmd.ExecuteSuperQueryListCallback != nil {
+		fmd.ExecuteSuperQueryListCallback()
+	}
 	for _, query := range queryList {
 		// test we still have a query to compare
 		if fmd.ExpectedExecuteSuperQueryCurrent >= len(fmd.ExpectedExecuteSuperQueryList) {

go/vt/mysqlctl/replication.go

@@ -352,7 +352,7 @@ func (mysqld *Mysqld) SetSuperReadOnly(ctx context.Context, on bool) (ResetSuper
 	} else {
 		query += "'OFF'"
 	}
-	if err := mysqld.ExecuteSuperQuery(context.Background(), query); err != nil {
+	if err := mysqld.ExecuteSuperQuery(ctx, query); err != nil {
 		return nil, err
 	}