From bbac419e7e8dd25c965de607744f9c15607521f0 Mon Sep 17 00:00:00 2001 From: Kevin Cao Date: Mon, 23 Dec 2024 15:33:31 -0500 Subject: [PATCH 1/2] 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 | 38 +++++++++++++++++++++++- 2 files changed, 54 insertions(+), 13 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..a2f5bcaf95ee 100644 --- a/pkg/backup/alter_backup_schedule_test.go +++ b/pkg/backup/alter_backup_schedule_test.go @@ -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) @@ -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) { From e53eb60ef032e2d1ee502bc7820828e46a25b2f3 Mon Sep 17 00:00:00 2001 From: Rebecca Taft Date: Fri, 27 Dec 2024 08:55:13 -0600 Subject: [PATCH 2/2] opt: wrap a couple of long lines This PR also serves to test a recent change to blathers. Fixes #138038 Release note: None --- pkg/sql/opt/memo/expr_format.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/sql/opt/memo/expr_format.go b/pkg/sql/opt/memo/expr_format.go index 1669466f7268..720db28d1ddf 100644 --- a/pkg/sql/opt/memo/expr_format.go +++ b/pkg/sql/opt/memo/expr_format.go @@ -1042,9 +1042,11 @@ func (f *ExprFmtCtx) formatScalarWithLabel( f.Buffer.WriteString(": ") } switch scalar.Op() { - case opt.ProjectionsOp, opt.AggregationsOp, opt.UniqueChecksOp, opt.FKChecksOp, opt.KVOptionsOp, opt.FastPathUniqueChecksOp: + case opt.ProjectionsOp, opt.AggregationsOp, opt.UniqueChecksOp, opt.FKChecksOp, opt.KVOptionsOp, + opt.FastPathUniqueChecksOp: // Omit empty lists (except filters) and special-purpose fast path check expressions. - if scalar.ChildCount() == 0 || (scalar.Op() == opt.FastPathUniqueChecksOp && f.HasFlags(ExprFmtHideFastPathChecks)) { + if scalar.ChildCount() == 0 || + (scalar.Op() == opt.FastPathUniqueChecksOp && f.HasFlags(ExprFmtHideFastPathChecks)) { return }