Merge #79121

79121: backupccl: allow user to run SHOW BACKUP on locality aware backups r=adityamaru,benbardin a=msbutler Previously, the user could only run SHOW BACKUP on the default locality of a backup. This patch allows users to pass in all backup localities, allowing future changes to SHOW BACKUP (e.g. #77694) to analyze data from backup files in non default locaties. Users can not yet run SHOW BACKUP for locality aware backups created using the incremental_location parameter. Informs #78632 Release note (sql change): users can pass locality aware backup URIs to SHOW BACKUP. This change only affects SHOW BACKUP with the new syntax: e.g.: SHOW BACKUP FROM LATEST IN (<collectionURI>, <localityURI1>, <localityURI2>) Users can not yet run SHOW BACKUP for locality aware backups created using the incremental_location parameter. Co-authored-by: Michael Butler <[email protected]>
cockroachdb · Apr 20, 2022 · 8a45881 · 8a45881
2 parents fc58be8 + acf60fb
commit 8a45881
Show file tree

Hide file tree

Showing 7 changed files with 307 additions and 186 deletions.
diff --git a/docs/generated/sql/bnf/show_backup.bnf b/docs/generated/sql/bnf/show_backup.bnf
@@ -1,11 +1,11 @@
 show_backup_stmt ::=
-	'SHOW' 'BACKUPS' 'IN' location
-	| 'SHOW' 'BACKUP' show_backup_details 'FROM' string_or_placeholder 'IN' string_or_placeholder 'WITH' kv_option_list
-	| 'SHOW' 'BACKUP' show_backup_details 'FROM' string_or_placeholder 'IN' string_or_placeholder 'WITH' 'OPTIONS' '(' kv_option_list ')'
-	| 'SHOW' 'BACKUP' show_backup_details 'FROM' string_or_placeholder 'IN' string_or_placeholder 
-	| 'SHOW' 'BACKUP' subdirectory 'IN' location 'WITH' kv_option_list
-	| 'SHOW' 'BACKUP' subdirectory 'IN' location 'WITH' 'OPTIONS' '(' kv_option_list ')'
-	| 'SHOW' 'BACKUP' subdirectory 'IN' location 
+	'SHOW' 'BACKUPS' 'IN' location_opt_list
+	| 'SHOW' 'BACKUP' show_backup_details 'FROM' string_or_placeholder 'IN' string_or_placeholder_opt_list 'WITH' kv_option_list
+	| 'SHOW' 'BACKUP' show_backup_details 'FROM' string_or_placeholder 'IN' string_or_placeholder_opt_list 'WITH' 'OPTIONS' '(' kv_option_list ')'
+	| 'SHOW' 'BACKUP' show_backup_details 'FROM' string_or_placeholder 'IN' string_or_placeholder_opt_list 
+	| 'SHOW' 'BACKUP' subdirectory 'IN' location_opt_list 'WITH' kv_option_list
+	| 'SHOW' 'BACKUP' subdirectory 'IN' location_opt_list 'WITH' 'OPTIONS' '(' kv_option_list ')'
+	| 'SHOW' 'BACKUP' subdirectory 'IN' location_opt_list 
 	| 'SHOW' 'BACKUP' string_or_placeholder 'WITH' kv_option_list
 	| 'SHOW' 'BACKUP' string_or_placeholder 'WITH' 'OPTIONS' '(' kv_option_list ')'
 	| 'SHOW' 'BACKUP' string_or_placeholder 

diff --git a/docs/generated/sql/bnf/stmt_block.bnf b/docs/generated/sql/bnf/stmt_block.bnf
@@ -699,9 +699,9 @@ use_stmt ::=
 	'USE' var_value
 
 show_backup_stmt ::=
-	'SHOW' 'BACKUPS' 'IN' string_or_placeholder
-	| 'SHOW' 'BACKUP' show_backup_details 'FROM' string_or_placeholder 'IN' string_or_placeholder opt_with_options
-	| 'SHOW' 'BACKUP' string_or_placeholder 'IN' string_or_placeholder opt_with_options
+	'SHOW' 'BACKUPS' 'IN' string_or_placeholder_opt_list
+	| 'SHOW' 'BACKUP' show_backup_details 'FROM' string_or_placeholder 'IN' string_or_placeholder_opt_list opt_with_options
+	| 'SHOW' 'BACKUP' string_or_placeholder 'IN' string_or_placeholder_opt_list opt_with_options
 	| 'SHOW' 'BACKUP' string_or_placeholder opt_with_options
 	| 'SHOW' 'BACKUP' 'SCHEMAS' string_or_placeholder opt_with_options
 	| 'SHOW' 'BACKUP' 'FILES' string_or_placeholder opt_with_options

diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go
@@ -650,6 +650,7 @@ func TestBackupAndRestoreJobDescription(t *testing.T) {
 	}
 
 	sqlDB.Exec(t, "BACKUP TO ($1, $2, $3)", backups...)
+	sqlDB.Exec(t, "BACKUP TO ($1,$2,$3) INCREMENTAL FROM $4", append(incrementals, backups[0])...)
 	sqlDB.Exec(t, "BACKUP INTO ($1, $2, $3)", collections...)
 	sqlDB.Exec(t, "BACKUP INTO LATEST IN ($1, $2, $3)", collections...)
 	sqlDB.Exec(t, "BACKUP INTO LATEST IN ($1, $2, $3) WITH incremental_location = ($4, $5, $6)",
@@ -665,6 +666,12 @@ func TestBackupAndRestoreJobDescription(t *testing.T) {
 	time.Sleep(time.Second + 2)
 	sqlDB.Exec(t, "BACKUP INTO ($1, $2, $3) AS OF SYSTEM TIME '-1s'", collections...)
 
+	{
+		// Ensure old style show backup runs properly with locality aware uri
+		sqlDB.Exec(t, "SHOW BACKUP $1", backups[0])
+		sqlDB.Exec(t, "SHOW BACKUP $1", incrementals[0])
+	}
+
 	// Find the subdirectory created by the full BACKUP INTO statement.
 	matches, err := filepath.Glob(path.Join(tmpDir, "full/*/*/*/"+backupManifestName))
 	require.NoError(t, err)
@@ -680,6 +687,8 @@ func TestBackupAndRestoreJobDescription(t *testing.T) {
 		[][]string{
 			{fmt.Sprintf("BACKUP TO ('%s', '%s', '%s')", backups[0].(string), backups[1].(string),
 				backups[2].(string))},
+			{fmt.Sprintf("BACKUP TO ('%s', '%s', '%s') INCREMENTAL FROM '%s'", incrementals[0],
+				incrementals[1], incrementals[2], backups[0])},
 			{fmt.Sprintf("BACKUP INTO '%s' IN ('%s', '%s', '%s')", full1, collections[0],
 				collections[1], collections[2])},
 			{fmt.Sprintf("BACKUP INTO '%s' IN ('%s', '%s', '%s')", full1,
@@ -9581,6 +9590,7 @@ func TestBackupRestoreOldIncrementalDefault(t *testing.T) {
 
 		sib := fmt.Sprintf("BACKUP DATABASE fkdb INTO LATEST IN %s WITH incremental_location = %s", dest, inc)
 		sqlDB.Exec(t, sib)
+
 		sir := fmt.Sprintf("RESTORE DATABASE fkdb FROM LATEST IN %s WITH new_db_name = 'inc_fkdb'", dest)
 		sqlDB.Exec(t, sir)
 
@@ -9690,6 +9700,39 @@ func TestBackupRestoreSeparateExplicitIsDefault(t *testing.T) {
 
 		sib := fmt.Sprintf("BACKUP DATABASE fkdb INTO LATEST IN %s WITH incremental_location = %s", dest, inc)
 		sqlDB.Exec(t, sib)
+		{
+			// Locality Aware Show Backup validation
+			// TODO (msbutler): move to data driven test after 22.1 backport
+
+			// Assert the localities field populates correctly (not null if backup is locality aware).
+			localities := sqlDB.QueryStr(t,
+				fmt.Sprintf("SELECT locality FROM [SHOW BACKUP FILES FROM LATEST IN %s]", dest))
+			expectedLocalities := map[string]bool{"default": true, "dc=dc1": true, "dc=dc2": true}
+			for _, locality := range localities {
+				if len(br.dest) > 1 {
+					_, ok := expectedLocalities[locality[0]]
+					require.Equal(t, true, ok)
+				} else {
+					require.Equal(t, "NULL", locality[0])
+				}
+			}
+			// Assert show backup still works.
+			sqlDB.Exec(t, fmt.Sprintf("SHOW BACKUPS IN %s", dest))
+			sqlDB.Exec(t, fmt.Sprintf("SHOW BACKUP FROM LATEST IN %s", dest))
+
+			if len(br.dest) > 1 {
+				// Locality aware show backups will eventually fail if not all localities are provided,
+				// but for now, they're ok.
+				sqlDB.Exec(t, fmt.Sprintf("SHOW BACKUP FROM LATEST IN %s", br.dest[1]))
+
+				errorMsg := "SHOW BACKUP on locality aware backups using incremental_location is not" +
+					" supported yet"
+				sqlDB.ExpectErr(t, errorMsg, fmt.Sprintf("SHOW BACKUP FROM LATEST IN %s WITH incremental_location= %s", dest, br.inc[0]))
+			} else {
+				// non locality aware show backup with incremental_location should work!
+				sqlDB.Exec(t, fmt.Sprintf("SHOW BACKUP FROM LATEST IN %s WITH incremental_location= %s", dest, inc))
+			}
+		}
 		sir := fmt.Sprintf("RESTORE DATABASE fkdb FROM LATEST IN %s WITH new_db_name = 'inc_fkdb', incremental_location = %s", dest, inc)
 		sqlDB.Exec(t, sir)
 

diff --git a/pkg/ccl/backupccl/manifest_handling.go b/pkg/ccl/backupccl/manifest_handling.go
@@ -629,6 +629,8 @@ func loadBackupManifests(
 	return backupManifests, memSize, nil
 }
 
+var errLocalityDescriptor = errors.New(`Locality Descriptor not found`)
+
 // getLocalityInfo takes a list of stores and their URIs, along with the main
 // backup manifest searches each for the locality pieces listed in the the
 // main manifest, returning the mapping.
@@ -672,7 +674,7 @@ func getLocalityInfo(
 			}
 		}
 		if !found {
-			return info, errors.Errorf("expected manifest %s not found in backup locations", filename)
+			return info, errors.Mark(errors.Newf("expected manifest %s not found in backup locations", filename), errLocalityDescriptor)
 		}
 	}
 	info.URIsByOriginalLocalityKV = urisByOrigLocality