Skip to content

Commit

Permalink
backup: fix alter backup schedule failing on uri with spaces
Browse files Browse the repository at this point in the history
`ALTER BACKUP SCHEDULE` being run on a backup schedule whose URI
contains spaces would fail with error `ERROR: parse
"'gs://cockroachdb-backup-testing/kevin spacey?AUTH=implicit'": first
path segment in URL cannot contain colon`.

This was caused by the fact that despite the `FmtBareStrings` flag being
used, the space character is considered one of the "special characters"
outlined in its doc that causes the string to remain wrapped with
quotes. Passing this quoted string to `url.Parse` fails.

Fixes: #134861

Release note (bug fix): `ALTER BACKUP SCHEDULE` no longer fails on
schedules whose collection URI contains a space.
  • Loading branch information
kev-cao committed Dec 26, 2024
1 parent 2df185d commit bbac419
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 13 deletions.
29 changes: 17 additions & 12 deletions pkg/backup/alter_backup_schedule.go
Original file line number Diff line number Diff line change
Expand Up @@ -213,30 +213,35 @@ func doAlterBackupSchedules(
return err
}

if err := emitAlteredSchedule(s.incJob, s.incStmt, resultsCh); err != nil {
if err := emitAlteredSchedule(ctx, p, 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)
return emitAlteredSchedule(ctx, p, s.fullJob, s.fullStmt, resultsCh)
}

func emitAlteredSchedule(
job *jobs.ScheduledJob, stmt *tree.Backup, resultsCh chan<- tree.Datums,
ctx context.Context,
p sql.PlanHookState,
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|tree.FmtShowFullURIs)
exprEval := p.ExprEvaluator("BACKUP")
to, err := exprEval.StringArray(ctx, tree.Exprs(stmt.To))
if err != nil {
return err
}
kmsURIs := make([]string, len(stmt.Options.EncryptionKMSURI))
for i, kmsURI := range stmt.Options.EncryptionKMSURI {
kmsURIs[i] = tree.AsStringWithFlags(kmsURI, tree.FmtBareStrings|tree.FmtShowFullURIs)
kmsURIs, err := exprEval.StringArray(ctx, tree.Exprs(stmt.Options.EncryptionKMSURI))
if err != nil {
return err
}
incDests := make([]string, len(stmt.Options.IncrementalStorage))
for i, incDest := range stmt.Options.IncrementalStorage {
incDests[i] = tree.AsStringWithFlags(incDest, tree.FmtBareStrings|tree.FmtShowFullURIs)
incDests, err := exprEval.StringArray(ctx, tree.Exprs(stmt.Options.IncrementalStorage))
if err != nil {
return err
}
if err := emitSchedule(job, stmt, to, kmsURIs, incDests, resultsCh); err != nil {
return err
Expand Down
38 changes: 37 additions & 1 deletion pkg/backup/alter_backup_schedule_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ func TestAlterBackupSchedulePausesIncrementalForNewCollection(t *testing.T) {
fullID, err := strconv.Atoi(rows[1][0])
require.NoError(t, err)

// Artificially resume inc schedule to test if it gets paused after the alter
// Artificially resume inc schedule to test if it gets paused after the alter.
th.sqlDB.Exec(t, `RESUME SCHEDULE $1`, incID)

alterCmd := fmt.Sprintf(`ALTER BACKUP SCHEDULE %d SET INTO 'nodelocal://1/backup/alter-schedule-2';`, fullID)
Expand All @@ -271,6 +271,42 @@ func TestAlterBackupSchedulePausesIncrementalForNewCollection(t *testing.T) {
require.Equal(t, "@daily", fullRecurrence)
}

func TestAlterBackupScheduleWithSQLSpecialCharacters(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

th, cleanup := newAlterSchedulesTestHelper(t, nil)
defer cleanup()

// Characters that require quoting as specified in mustQuoteMap in
// sql/lexbase/encode.go.
uri := "nodelocal://1/backup/alter ,s{hedu}e"

createCmd := fmt.Sprintf(
"CREATE SCHEDULE FOR BACKUP INTO '%s' WITH"+
" incremental_location = '%s' RECURRING '@hourly' FULL BACKUP '@daily';",
uri, uri,
)
rows := th.sqlDB.QueryStr(t, createCmd)
require.Len(t, rows, 2)
incID, err := strconv.Atoi(rows[0][0])
require.NoError(t, err)
fullID, err := strconv.Atoi(rows[1][0])
require.NoError(t, err)

alterCmd := fmt.Sprintf(
"ALTER BACKUP SCHEDULE %d SET INTO '%s', "+
"SET RECURRING '@daily', SET FULL BACKUP '@weekly';",
fullID, uri,
)
th.sqlDB.Exec(t, alterCmd)

_, incRecurrence := scheduleStatusAndRecurrence(t, th, incID)
_, fullRecurrence := scheduleStatusAndRecurrence(t, th, fullID)
require.Equal(t, "@daily", incRecurrence)
require.Equal(t, "@weekly", fullRecurrence)
}

func scheduleStatusAndRecurrence(
t *testing.T, th *alterSchedulesTestHelper, id int,
) (status string, recurrence string) {
Expand Down

0 comments on commit bbac419

Please sign in to comment.