From b90e28c63b146a9b155c93364bac5c3bd2daa052 Mon Sep 17 00:00:00 2001
From: Kevin Cao <kevin.cao@cockroachlabs.com>
Date: Mon, 23 Dec 2024 15:33:31 -0500
Subject: [PATCH] backup: fix alter backup schedule failing on uri with spaces

`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.
---
 pkg/backup/alter_backup_schedule.go      | 29 +++++++++++++----------
 pkg/backup/alter_backup_schedule_test.go | 30 ++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/pkg/backup/alter_backup_schedule.go b/pkg/backup/alter_backup_schedule.go
index 50e05e814f0a..6bcfcfe30667 100644
--- a/pkg/backup/alter_backup_schedule.go
+++ b/pkg/backup/alter_backup_schedule.go
@@ -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
diff --git a/pkg/backup/alter_backup_schedule_test.go b/pkg/backup/alter_backup_schedule_test.go
index 75a8a4981efb..1761f9bf6c47 100644
--- a/pkg/backup/alter_backup_schedule_test.go
+++ b/pkg/backup/alter_backup_schedule_test.go
@@ -271,6 +271,36 @@ 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' RECURRING '@hourly' FULL BACKUP '@daily';", 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 RECURRING '@daily', SET FULL BACKUP '@weekly';`,
+		fullID,
+	)
+	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) {