Skip to content

Commit

Permalink
backupccl: fill incremental cluster id on alter schedule
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
kev-cao committed Sep 23, 2024
1 parent d644373 commit f943f81
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 1 deletion.
10 changes: 9 additions & 1 deletion pkg/ccl/backupccl/alter_backup_schedule.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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)
Expand All @@ -476,7 +484,7 @@ func processFullBackupRecurrence(
p.User(),
s.fullJob.ScheduleLabel(),
incRecurrence,
*s.fullJob.ScheduleDetails(),
schedDetails,
jobspb.InvalidScheduleID,
s.fullArgs.UpdatesLastBackupMetric,
s.incStmt,
Expand Down
37 changes: 37 additions & 0 deletions pkg/ccl/backupccl/alter_backup_schedule_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

0 comments on commit f943f81

Please sign in to comment.