diff --git a/pkg/ccl/backupccl/backupdest/incrementals.go b/pkg/ccl/backupccl/backupdest/incrementals.go index ea1582ecc3d6..769760bacf71 100644 --- a/pkg/ccl/backupccl/backupdest/incrementals.go +++ b/pkg/ccl/backupccl/backupdest/incrementals.go @@ -32,21 +32,41 @@ const ( // backupSubdirRE identifies the portion of a larger path that refers to the full backup subdirectory. var backupSubdirRE = regexp.MustCompile(`(.*)/([0-9]{4}/[0-9]{2}/[0-9]{2}-[0-9]{6}.[0-9]{2}/?)$`) -// CollectionAndSubdir breaks up a path into those components, if applicable. -// "Specific" commands, like BACKUP INTO and RESTORE FROM, don't need this. -// "Vague" commands, like SHOW BACKUP and debug backup, sometimes do. -func CollectionAndSubdir(path string, subdir string) (string, string) { +// CollectionsAndSubdir breaks up the given paths into those +// components, if applicable. An error returned if the matched +// subdirectory is different between paths. +// +// "Specific" commands, like BACKUP INTO and RESTORE FROM, don't need +// this. "Vague" commands, like SHOW BACKUP and debug backup, +// sometimes do. +func CollectionsAndSubdir(paths []string, subdir string) ([]string, string, error) { if subdir != "" { - return path, subdir + return paths, subdir, nil } // Split out the backup name from the base directory so we can search the // default "incrementals" subdirectory. - matchResult := backupSubdirRE.FindStringSubmatch(path) - if matchResult == nil { - return path, subdir + // + // NOTE(ssd): I am unaware of a way to get to this point + // without an explicit subdir but with multiple paths. + output := make([]string, len(paths)) + var matchedSubdirectory string + for i, p := range paths { + matchResult := backupSubdirRE.FindStringSubmatch(p) + if matchResult == nil { + return paths, matchedSubdirectory, nil + } + output[i] = matchResult[1] + if matchedSubdirectory == "" { + matchedSubdirectory = matchResult[2] + } + if matchedSubdirectory != matchResult[2] { + return paths, matchedSubdirectory, errors.Newf("provided backup locations appear to reference different full backups: %s and %s", + matchedSubdirectory, + matchResult[2]) + } } - return matchResult[1], matchResult[2] + return output, matchedSubdirectory, nil } // FindPriorBackups finds "appended" incremental backups by searching diff --git a/pkg/ccl/backupccl/backupdest/incrementals_test.go b/pkg/ccl/backupccl/backupdest/incrementals_test.go index 6c9a1f00987d..720f7dfb2aba 100644 --- a/pkg/ccl/backupccl/backupdest/incrementals_test.go +++ b/pkg/ccl/backupccl/backupdest/incrementals_test.go @@ -11,12 +11,98 @@ package backupdest_test import ( "testing" + "github.com/cockroachdb/cockroach/pkg/ccl/backupccl/backupdest" "github.com/cockroachdb/cockroach/pkg/ccl/backupccl/backuputils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/stretchr/testify/require" ) +func TestCollectionsAndSubdir(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + type testcase struct { + name string + paths []string + subdir string + expectedCollections []string + expectedSubdir string + expectedError string + } + testcases := []testcase{ + { + name: "non-empty subdir returns unmodified collection", + paths: []string{"nodelocal://1/backup-dest/2023/05/10-160331.72/"}, + subdir: "foo", + expectedCollections: []string{"nodelocal://1/backup-dest/2023/05/10-160331.72/"}, + expectedSubdir: "foo", + }, + { + name: "non-empty subdir returns all parts of unmodified collection", + paths: []string{ + "nodelocal://1/backup-dest/", + "nodelocal://2/backup-dest/", + }, + subdir: "foo", + expectedCollections: []string{ + "nodelocal://1/backup-dest/", + "nodelocal://2/backup-dest/", + }, + expectedSubdir: "foo", + }, + { + name: "date-based-path is returned as subdir if no subdir is provided", + paths: []string{"nodelocal://1/backup-dest/2023/05/10-160331.72/"}, + expectedCollections: []string{"nodelocal://1/backup-dest"}, + expectedSubdir: "2023/05/10-160331.72/", + }, + { + name: "multiple date-based paths are returned as subdir if no subdir is provided", + paths: []string{ + "nodelocal://1/backup-dest/2023/05/10-160331.72/", + "nodelocal://2/backup-dest/2023/05/10-160331.72/", + }, + expectedCollections: []string{ + "nodelocal://1/backup-dest", + "nodelocal://2/backup-dest", + }, + expectedSubdir: "2023/05/10-160331.72/", + }, + { + name: "paths that don't match are returned unmodified", + paths: []string{ + "nodelocal://1/backup-dest/2023/05/9999/", + "nodelocal://2/backup-dest/2023/05/9999/", + }, + expectedCollections: []string{ + "nodelocal://1/backup-dest/2023/05/9999/", + "nodelocal://2/backup-dest/2023/05/9999/", + }, + expectedSubdir: "", + }, + { + name: "different date-based paths results in an error", + paths: []string{ + "nodelocal://1/backup-dest/2023/05/10-160331.72/", + "nodelocal://2/backup-dest/2023/05/10-160331.73/", + }, + expectedError: "provided backup locations appear to reference different full backups", + }, + } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + collections, subdir, err := backupdest.CollectionsAndSubdir(tc.paths, tc.subdir) + if tc.expectedError != "" { + require.ErrorContains(t, err, tc.expectedError) + } else { + require.Equal(t, tc.expectedSubdir, subdir) + require.Equal(t, tc.expectedCollections, collections) + } + }) + } +} + func TestJoinURLPath(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) diff --git a/pkg/ccl/backupccl/show.go b/pkg/ccl/backupccl/show.go index 186fe2ff0952..fc88f7c0fbd1 100644 --- a/pkg/ccl/backupccl/show.go +++ b/pkg/ccl/backupccl/show.go @@ -370,7 +370,7 @@ func showBackupPlanHook( p.ExecCfg().InternalDB, p.User(), ) - showEncErr := `If you are running SHOW BACKUP exclusively on an incremental backup, + showEncErr := `If you are running SHOW BACKUP exclusively on an incremental backup, you must pass the 'encryption_info_dir' parameter that points to the directory of your full backup` if showStmt.Options.EncryptionPassphrase != nil { passphrase, err := exprEval.String(ctx, showStmt.Options.EncryptionPassphrase) @@ -428,13 +428,16 @@ you must pass the 'encryption_info_dir' parameter that points to the directory o return err } } - collection, computedSubdir := backupdest.CollectionAndSubdir(dest[0], subdir) + collections, computedSubdir, err := backupdest.CollectionsAndSubdir(dest, subdir) + if err != nil { + return err + } fullyResolvedIncrementalsDirectory, err := backupdest.ResolveIncrementalsBackupLocation( ctx, p.User(), p.ExecCfg(), explicitIncPaths, - []string{collection}, + collections, computedSubdir, ) if err != nil { diff --git a/pkg/ccl/backupccl/testdata/backup-restore/show-backup-multiregion b/pkg/ccl/backupccl/testdata/backup-restore/show-backup-multiregion index 49b089711264..cdfd5f56c88a 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/show-backup-multiregion +++ b/pkg/ccl/backupccl/testdata/backup-restore/show-backup-multiregion @@ -39,3 +39,25 @@ SELECT * FROM [SHOW REGIONS FROM DATABASE d] ORDER BY region; d eu-central-1 false false {eu-central-1} d us-east-1 true false {us-east1} d us-west-1 false false {us-west1} + +# Now create a locality aware backup and ensure that the SHOW command correctly shows the +# backup. +exec-sql +BACKUP DATABASE d INTO ('nodelocal://1/database_backup_locality_aware_default?COCKROACH_LOCALITY=default', 'nodelocal://1/database_backup_locality_aware_west?COCKROACH_LOCALITY=region=us-west-1', 'nodelocal://1/database_backup_locality_aware_central?COCKROACH_LOCALITY=region=eu-central-1'); +---- + +exec-sql +BACKUP DATABASE d INTO LATEST IN ('nodelocal://1/database_backup_locality_aware_default?COCKROACH_LOCALITY=default', 'nodelocal://1/database_backup_locality_aware_west?COCKROACH_LOCALITY=region=us-west-1', 'nodelocal://1/database_backup_locality_aware_central?COCKROACH_LOCALITY=region=eu-central-1'); +---- + +query-sql +SELECT object_name, object_type, backup_type FROM [SHOW BACKUP LATEST IN ('nodelocal://1/database_backup_locality_aware_default?COCKROACH_LOCALITY=default', 'nodelocal://1/database_backup_locality_aware_west?COCKROACH_LOCALITY=region=us-west-1', 'nodelocal://1/database_backup_locality_aware_central?COCKROACH_LOCALITY=region=eu-central-1')]; +---- +d database full +public schema full +crdb_internal_region type full +_crdb_internal_region type full +d database incremental +public schema incremental +crdb_internal_region type incremental +_crdb_internal_region type incremental