Merge cockroachdb#86819

86819: backupccl: add planning validation to ALTER BACKUP SCHEDULE, emit results r=benbardin a=benbardin

Release note (enterprise change): Alter Backup Schedule will fail if the new backup statement does not pass planning.

Release justification: Improving reliability on feature launching for 22.2.

Co-authored-by: Ben Bardin <[email protected]>

pkg/ccl/backupccl/BUILD.bazel

@@ -147,6 +147,7 @@ go_test(
     name = "backupccl_test",
     size = "enormous",
     srcs = [
+        "alter_backup_schedule_test.go",
         "alter_backup_test.go",
         "backup_cloud_test.go",
         "backup_intents_test.go",

pkg/ccl/backupccl/alter_backup_schedule.go

@@ -164,7 +164,11 @@ func doAlterBackupSchedules(
 		return err
 	}
 
-	// TODO(benbardin): Verify backup statement.
+	// Run full backup in dry-run mode.  This will do all of the sanity checks
+	// and validation we need to make in order to ensure the schedule is sane.
+	if _, err = dryRunBackup(ctx, p, s.fullStmt); err != nil {
+		return errors.Wrap(err, "failed to dry run backup")
+	}
 
 	s.fullArgs.BackupStatement = tree.AsStringWithFlags(s.fullStmt, tree.FmtParsable|tree.FmtShowPasswords)
 	fullAny, err := pbtypes.MarshalAny(s.fullArgs)
@@ -190,8 +194,36 @@ func doAlterBackupSchedules(
 		if err := s.incJob.Update(ctx, p.ExecCfg().InternalExecutor, p.Txn()); err != nil {
 			return err
 		}
+
+		if err := emitAlteredSchedule(s.incJob, s.incStmt, resultsCh); err != nil {
+			return err
+		}
+	}
+
+	// Emit the full backup schedule after the incremental.
+	// This matches behavior in CREATE SCHEDULE FOR BACKUP.
+	return emitAlteredSchedule(s.fullJob, s.fullStmt, resultsCh)
+}
+
+func emitAlteredSchedule(
+	job *jobs.ScheduledJob, stmt *tree.Backup, resultsCh chan<- tree.Datums,
+) error {
+	to := make([]string, len(stmt.To))
+	for i, dest := range stmt.To {
+		to[i] = tree.AsStringWithFlags(dest, tree.FmtBareStrings)
+	}
+	kmsURIs := make([]string, len(stmt.Options.EncryptionKMSURI))
+	for i, kmsURI := range stmt.Options.EncryptionKMSURI {
+		kmsURIs[i] = tree.AsStringWithFlags(kmsURI, tree.FmtBareStrings)
+	}
+	incDests := make([]string, len(stmt.Options.IncrementalStorage))
+	for i, incDest := range stmt.Options.IncrementalStorage {
+		incDests[i] = tree.AsStringWithFlags(incDest, tree.FmtBareStrings)
+	}
+	if err := emitSchedule(job, stmt, to, nil, /* incrementalFrom */
+		kmsURIs, incDests, resultsCh); err != nil {
+		return err
 	}
-	// TODO(benbardin): Emit schedules.
 	return nil
 }
 

pkg/ccl/backupccl/alter_backup_schedule_test.go

@@ -0,0 +1,144 @@
+// Copyright 2020 The Cockroach Authors.
+//
+// Licensed as a CockroachDB Enterprise file under the Cockroach Community
+// License (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+//     https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt
+
+package backupccl
+
+import (
+	"context"
+	"fmt"
+	"testing"
+	"time"
+
+	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/jobs"
+	"github.com/cockroachdb/cockroach/pkg/jobs/jobstest"
+	"github.com/cockroachdb/cockroach/pkg/scheduledjobs"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
+	"github.com/stretchr/testify/require"
+)
+
+// alterSchedulesTestHelper starts a server, and arranges for job scheduling daemon to
+// use jobstest.JobSchedulerTestEnv.
+// This helper also arranges for the manual override of scheduling logic
+// via executeSchedules callback.
+type execAlterSchedulesFn = func(ctx context.Context, maxSchedules int64) error
+type alterSchedulesTestHelper struct {
+	iodir            string
+	server           serverutils.TestServerInterface
+	env              *jobstest.JobSchedulerTestEnv
+	cfg              *scheduledjobs.JobExecutionConfig
+	sqlDB            *sqlutils.SQLRunner
+	executeSchedules func() error
+}
+
+// newAlterSchedulesTestHelper creates and initializes appropriate state for a test,
+// returning alterSchedulesTestHelper as well as a cleanup function.
+func newAlterSchedulesTestHelper(t *testing.T) (*alterSchedulesTestHelper, func()) {
+	dir, dirCleanupFn := testutils.TempDir(t)
+
+	th := &alterSchedulesTestHelper{
+		env: jobstest.NewJobSchedulerTestEnv(
+			jobstest.UseSystemTables, timeutil.Now(), tree.ScheduledBackupExecutor),
+		iodir: dir,
+	}
+
+	knobs := &jobs.TestingKnobs{
+		JobSchedulerEnv: th.env,
+		TakeOverJobsScheduling: func(fn execAlterSchedulesFn) {
+			th.executeSchedules = func() error {
+				defer th.server.JobRegistry().(*jobs.Registry).TestingNudgeAdoptionQueue()
+				return fn(context.Background(), allSchedules)
+			}
+		},
+		CaptureJobExecutionConfig: func(config *scheduledjobs.JobExecutionConfig) {
+			th.cfg = config
+		},
+		IntervalOverrides: jobs.NewTestingKnobsWithShortIntervals().IntervalOverrides,
+	}
+
+	args := base.TestServerArgs{
+		ExternalIODir: dir,
+		// Some scheduled backup tests fail when run within a tenant. More
+		// investigation is required. Tracked with #76378.
+		DisableDefaultTestTenant: true,
+		Knobs: base.TestingKnobs{
+			JobsTestingKnobs: knobs,
+		},
+	}
+	s, db, _ := serverutils.StartServer(t, args)
+	require.NotNil(t, th.cfg)
+	th.sqlDB = sqlutils.MakeSQLRunner(db)
+	th.server = s
+	th.sqlDB.Exec(t, `SET CLUSTER SETTING bulkio.backup.merge_file_buffer_size = '1MiB'`)
+	th.sqlDB.Exec(t, `SET CLUSTER SETTING kv.closed_timestamp.target_duration = '100ms'`) // speeds up test
+
+	return th, func() {
+		dirCleanupFn()
+		s.Stopper().Stop(context.Background())
+	}
+}
+
+func TestAlterBackupScheduleEmitsSummary(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	th, cleanup := newAlterSchedulesTestHelper(t)
+	defer cleanup()
+
+	th.sqlDB.Exec(t, `
+CREATE DATABASE mydb;
+USE mydb;
+
+CREATE TABLE t1(a int);
+INSERT INTO t1 values (1), (10), (100);
+`)
+
+	rows := th.sqlDB.Query(t,
+		`CREATE SCHEDULE FOR BACKUP t1 INTO 'nodelocal://0/backup/alter-schedule' RECURRING '@daily';`)
+	require.NoError(t, rows.Err())
+
+	var scheduleID int64
+	var unusedStr string
+	var unusedTS *time.Time
+	rowCount := 0
+	for rows.Next() {
+		// We just need to retrieve one of the schedule IDs, don't care whether
+		// it's the incremental or full.
+		require.NoError(t, rows.Scan(&scheduleID, &unusedStr, &unusedStr, &unusedTS, &unusedStr, &unusedStr))
+		rowCount++
+	}
+	require.Equal(t, 2, rowCount)
+
+	var status, schedule, backupStmt string
+	var statuses, schedules, backupStmts []string
+	rows = th.sqlDB.Query(t,
+		fmt.Sprintf(`ALTER BACKUP SCHEDULE %d SET FULL BACKUP '@weekly';`, scheduleID))
+	require.NoError(t, rows.Err())
+	for rows.Next() {
+		require.NoError(t, rows.Scan(&scheduleID, &unusedStr, &status, &unusedTS, &schedule, &backupStmt))
+		statuses = append(statuses, status)
+		schedules = append(schedules, schedule)
+		backupStmts = append(backupStmts, backupStmt)
+	}
+
+	// Incremental should be emitted first.
+	require.Equal(t, []string{"PAUSED: Waiting for initial backup to complete", "ACTIVE"}, statuses)
+	require.Equal(t, []string{"@daily", "@weekly"}, schedules)
+	require.Equal(t, []string{
+		"BACKUP TABLE mydb.public.t1 INTO LATEST IN 'nodelocal://0/backup/alter-schedule' WITH detached",
+		"BACKUP TABLE mydb.public.t1 INTO 'nodelocal://0/backup/alter-schedule' WITH detached",
+	},
+		backupStmts)
+
+}

pkg/ccl/backupccl/backup_planning.go

@@ -224,9 +224,10 @@ func GetRedactedBackupNode(
 	hasBeenPlanned bool,
 ) (*tree.Backup, error) {
 	b := &tree.Backup{
-		AsOf:    backup.AsOf,
-		Targets: backup.Targets,
-		Nested:  backup.Nested,
+		AsOf:           backup.AsOf,
+		Targets:        backup.Targets,
+		Nested:         backup.Nested,
+		AppendToLatest: backup.AppendToLatest,
 	}
 
 	// We set Subdir to the directory resolved during BACKUP planning.