Merge #103096

103096: backupccl: support locality-aware incremental backups in SHOW BACKUP r=msbutler a=stevendanna

Previously, SHOW BACKUP would only resolve incremental locations for the first collection URI passed to SHOW BACKUP. As a result, SHOW BACKUP on a backup that contained incrementals would result in an error such as

    ERROR: expected manifest
    /20230510/160359.17/BACKUP_PART_1_zone=us-east not found in backup
    locations

This only affected `SHOW` because of code in `SHOW` designed to support the user passing the URI to a specific full backup rather than a collection and a subdirectory.

This commit changes the CollectionAndSubdir function to support an array of backup locations.

Epic: none

Release note: Fixes a bug in which SHOW BACKUP would fail to show a locality-aware backup that contained incremental backups.

Co-authored-by: Steven Danna <[email protected]>

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

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)

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 {

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