From f943f818278024d2d8a12e606d52562d0fc9e664 Mon Sep 17 00:00:00 2001
From: Kevin Cao <kevin.cao@cockroachlabs.com>
Date: Mon, 23 Sep 2024 14:10:57 -0400
Subject: [PATCH] backupccl: fill incremental cluster id on alter schedule

When altering the recurrence of a singleton full backup schedule created
before v23.2, the corresponding new incremental schedule creation will
fail due to a missing cluster ID. This patch ensures that the cluster ID
is set when creating the incremental.

Fixes: #131127

Release note (bug fix): Fixed a bug introduced in v23.2.0 where creating a new
incremental schedule via `ALTER SCHEDULE` on a full backup schedule created on
an older version would fail.
---
 pkg/ccl/backupccl/alter_backup_schedule.go    | 10 ++++-
 .../backupccl/alter_backup_schedule_test.go   | 37 +++++++++++++++++++
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/pkg/ccl/backupccl/alter_backup_schedule.go b/pkg/ccl/backupccl/alter_backup_schedule.go
index 4ff80bc47645..7bdabce2d71d 100644
--- a/pkg/ccl/backupccl/alter_backup_schedule.go
+++ b/pkg/ccl/backupccl/alter_backup_schedule.go
@@ -27,6 +27,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
+	"github.com/cockroachdb/cockroach/pkg/util/uuid"
 	"github.com/cockroachdb/errors"
 	pbtypes "github.com/gogo/protobuf/types"
 )
@@ -464,6 +465,13 @@ func processFullBackupRecurrence(
 		s.incStmt = &tree.Backup{}
 		*s.incStmt = *s.fullStmt
 		s.incStmt.AppendToLatest = true
+		// Pre 23.2 schedules did not have a cluster ID, so if we are altering a
+		// schedule that was created before 23.2, we need to set the cluster ID on
+		// the newly created incremental manually.
+		schedDetails := *s.fullJob.ScheduleDetails()
+		if schedDetails.ClusterID.Equal(uuid.Nil) {
+			schedDetails.ClusterID = p.ExtendedEvalContext().ClusterID
+		}
 
 		rec := s.fullJob.ScheduleExpr()
 		incRecurrence, err := schedulebase.ComputeScheduleRecurrence(env.Now(), &rec)
@@ -476,7 +484,7 @@ func processFullBackupRecurrence(
 			p.User(),
 			s.fullJob.ScheduleLabel(),
 			incRecurrence,
-			*s.fullJob.ScheduleDetails(),
+			schedDetails,
 			jobspb.InvalidScheduleID,
 			s.fullArgs.UpdatesLastBackupMetric,
 			s.incStmt,
diff --git a/pkg/ccl/backupccl/alter_backup_schedule_test.go b/pkg/ccl/backupccl/alter_backup_schedule_test.go
index ed20d00aa92b..da53ddbb2fc5 100644
--- a/pkg/ccl/backupccl/alter_backup_schedule_test.go
+++ b/pkg/ccl/backupccl/alter_backup_schedule_test.go
@@ -158,5 +158,42 @@ INSERT INTO t1 values (1), (10), (100);
 
 	rows = th.sqlDB.QueryStr(t, fmt.Sprintf(`ALTER BACKUP SCHEDULE %d EXECUTE IMMEDIATELY;`, scheduleID))
 	require.Equal(t, trim(th.env.Now().String()), trim(rows[0][3]))
+}
+
+func TestAlterBackupScheduleSetsIncrementalClusterID(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	th, cleanup := newAlterSchedulesTestHelper(t, nil)
+	defer cleanup()
 
+	rows := th.sqlDB.QueryStr(
+		t,
+		`CREATE SCHEDULE FOR BACKUP INTO 'nodelocal://1/backup/alter-schedule' RECURRING '@daily' FULL BACKUP ALWAYS;`,
+	)
+	require.Len(t, rows, 1)
+	scheduleID, err := strconv.Atoi(rows[0][0])
+	require.NoError(t, err)
+
+	// Artificially remove cluster ID from full backup to simulate pre-23.2 schedule.
+	th.sqlDB.QueryStr(
+		t,
+		fmt.Sprintf(`UPDATE system.scheduled_jobs
+			SET
+			schedule_details = crdb_internal.json_to_pb(
+				'cockroach.jobs.jobspb.ScheduleDetails',
+				json_remove_path(
+					crdb_internal.pb_to_json('cockroach.jobs.jobspb.ScheduleDetails', schedule_details),
+					ARRAY['clusterId']
+				)
+			)
+			WHERE schedule_id=%d;`, scheduleID),
+	)
+
+	// Ensure creating incremental from a full backup schedule without a cluster ID passes
+	rows = th.sqlDB.QueryStr(t, fmt.Sprintf(
+		`ALTER BACKUP SCHEDULE %d SET RECURRING '@hourly', SET FULL BACKUP '@daily'`,
+		scheduleID),
+	)
+	require.Len(t, rows, 2)
 }