Merge #127970

127970: backupccl: update restore job description r=msbutler a=azhu-crl Previously, restore job description follows the old syntax `RESTORE ... FROM`, this PR updates the job description such that it reflects the new syntax: `RESTORE FROM {backup} IN {collectionURI}`. Epic: none Fixes: #127728 Release note (bug fix): Updated restore job description from `RESTORE ... FROM` to `RESTORE FROM {backup} IN {collectionURI}` to reflect the new restore syntax. Co-authored-by: Anne Zhu <anne.zhu@cockroachlabs.com>
cockroachdb · Jul 31, 2024 · 7afbca4 · 7afbca4
2 parents 0aaf45c + 7f7a528
commit 7afbca4
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 47 deletions.
diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go
@@ -820,41 +820,19 @@ func TestBackupAndRestoreJobDescription(t *testing.T) {
 	sqlDB.Exec(t, "DROP DATABASE data CASCADE")
 	sqlDB.Exec(t, "RESTORE DATABASE data FROM $4 IN ($1, $2, $3)", append(collections, asOf1)...)
 
-	// The flavors of BACKUP and RESTORE which automatically resolve the right
-	// directory to read/write data to, have URIs with the resolved path written
-	// to the job description.
-	getResolvedCollectionURIs := func(prefixes []interface{}, subdir string) []string {
-		resolvedCollectionURIs := make([]string, len(prefixes))
-		for i, collection := range prefixes {
-			parsed, err := url.Parse(collection.(string))
-			require.NoError(t, err)
-			parsed.Path = path.Join(parsed.Path, subdir)
-			resolvedCollectionURIs[i] = parsed.String()
-		}
-
-		return resolvedCollectionURIs
-	}
-
-	resolvedCollectionURIs := getResolvedCollectionURIs(collections, full1)
-	resolvedIncURIs := getResolvedCollectionURIs(incrementals, full1)
-	resolvedAsOfCollectionURIs := getResolvedCollectionURIs(collections, asOf1)
-
 	sqlDB.CheckQueryResults(
 		t, "SELECT description FROM crdb_internal.jobs WHERE job_type='RESTORE' ORDER BY created",
 		[][]string{
-			{fmt.Sprintf("RESTORE DATABASE data FROM ('%s', '%s', '%s')",
-				resolvedCollectionURIs[0], resolvedCollectionURIs[1],
-				resolvedCollectionURIs[2])},
-			{fmt.Sprintf("RESTORE DATABASE data FROM ('%s', '%s', '%s') WITH OPTIONS (incremental_location = ('%s', '%s', '%s'))",
-				resolvedCollectionURIs[0], resolvedCollectionURIs[1], resolvedCollectionURIs[2],
-				resolvedIncURIs[0], resolvedIncURIs[1], resolvedIncURIs[2])},
-			{fmt.Sprintf("RESTORE DATABASE data FROM ('%s', '%s', '%s')",
-				resolvedAsOfCollectionURIs[0], resolvedAsOfCollectionURIs[1],
-				resolvedAsOfCollectionURIs[2])},
+			{fmt.Sprintf("RESTORE DATABASE data FROM '%s' IN ('%s', '%s', '%s')",
+				full1, collections[0], collections[1], collections[2])},
+			{fmt.Sprintf("RESTORE DATABASE data FROM '%s' IN ('%s', '%s', '%s') WITH OPTIONS (incremental_location = ('%s', '%s', '%s'))",
+				full1, collections[0], collections[1], collections[2],
+				incrementals[0], incrementals[1], incrementals[2])},
+			{fmt.Sprintf("RESTORE DATABASE data FROM '%s' IN ('%s', '%s', '%s')",
+				asOf1, collections[0], collections[1], collections[2])},
 			// and again from LATEST IN...
-			{fmt.Sprintf("RESTORE DATABASE data FROM ('%s', '%s', '%s')",
-				resolvedAsOfCollectionURIs[0], resolvedAsOfCollectionURIs[1],
-				resolvedAsOfCollectionURIs[2])},
+			{fmt.Sprintf("RESTORE DATABASE data FROM '%s' IN ('%s', '%s', '%s')",
+				asOf1, collections[0], collections[1], collections[2])},
 		},
 	)
 }
@@ -1238,13 +1216,12 @@ func TestBackupRestoreSystemJobs(t *testing.T) {
 
 	sqlDB.Exec(t, `RESTORE TABLE bank FROM LATEST IN $1 WITH OPTIONS (into_db='restoredb')`, collectionDir)
 
-	//TODO(kev-cao): fix the restore description when restoring from a backup tree
-	buggyRestoreDescription := fmt.Sprintf("%s/full%s?AWS_SESSION_TOKEN=redacted", localFoo, fullDir)
 	if err := jobutils.VerifySystemJob(t, sqlDB, 0, jobspb.TypeRestore, jobs.StatusSucceeded, jobs.Record{
 		Username: username.RootUserName(),
 		Description: fmt.Sprintf(
-			`RESTORE TABLE bank FROM '%s' WITH OPTIONS (into_db = 'restoredb')`,
-			buggyRestoreDescription,
+			`RESTORE TABLE bank FROM '%s' IN '%sredacted' WITH OPTIONS (into_db = 'restoredb')`,
+			fullDir,
+			sanitizedCollectionDir,
 		),
 	}); err != nil {
 		t.Fatal(err)
@@ -1347,8 +1324,8 @@ into_db='restoredb', %s)`, encryptionOption), backupLoc1)
 			if err := jobutils.VerifySystemJob(t, sqlDB, 0, jobspb.TypeRestore, jobs.StatusSucceeded, jobs.Record{
 				Username: username.RootUserName(),
 				Description: fmt.Sprintf(
-					`RESTORE TABLE data.bank FROM '%s%s' WITH OPTIONS (%s, into_db = 'restoredb')`,
-					backupLoc1, fullDir, sanitizedEncryptionOption,
+					`RESTORE TABLE data.bank FROM '%s' IN '%s' WITH OPTIONS (%s, into_db = 'restoredb')`,
+					fullDir, backupLoc1, sanitizedEncryptionOption,
 				),
 				DescriptorIDs: descpb.IDs{
 					descpb.ID(restoreDatabaseID + 1),
@@ -5759,7 +5736,7 @@ func TestBackupRestoreShowJob(t *testing.T) {
 		t, "SELECT description FROM crdb_internal.jobs WHERE job_type = 'BACKUP' OR job_type = 'RESTORE' ORDER BY description",
 		[][]string{
 			{fmt.Sprintf("BACKUP DATABASE data INTO '%s' IN 'nodelocal://1/foo' WITH OPTIONS (revision_history = true)", fullDir)},
-			{fmt.Sprintf("RESTORE TABLE data.bank FROM 'nodelocal://1/foo%s' WITH OPTIONS (into_db = 'data 2', skip_missing_foreign_keys)", fullDir)},
+			{fmt.Sprintf("RESTORE TABLE data.bank FROM '%s' IN 'nodelocal://1/foo' WITH OPTIONS (into_db = 'data 2', skip_missing_foreign_keys)", fullDir)},
 		},
 	)
 }

diff --git a/pkg/ccl/backupccl/restore_planning.go b/pkg/ccl/backupccl/restore_planning.go
@@ -1129,12 +1129,14 @@ func restoreJobDescription(
 	intoDB string,
 	newDBName string,
 	kmsURIs []string,
+	resolvedSubdir string,
 ) (string, error) {
 	r := &tree.Restore{
 		DescriptorCoverage: restore.DescriptorCoverage,
 		AsOf:               restore.AsOf,
 		Targets:            restore.Targets,
 		From:               make([]tree.StringOrPlaceholderOptList, len(restore.From)),
+		Subdir:             tree.NewDString("/" + strings.TrimPrefix(resolvedSubdir, "/")),
 	}
 
 	var options tree.RestoreOptions
@@ -2037,12 +2039,6 @@ func doRestorePlan(
 	if err != nil {
 		return err
 	}
-	var fromDescription [][]string
-	if len(from) == 1 {
-		fromDescription = [][]string{fullyResolvedBaseDirectory}
-	} else {
-		fromDescription = from
-	}
 
 	if restoreStmt.Options.ExperimentalOnline {
 		if err := checkBackupElidedPrefixForOnlineCompat(ctx, mainBackupManifests, descriptorRewrites); err != nil {
@@ -2054,12 +2050,13 @@ func doRestorePlan(
 		ctx,
 		p,
 		restoreStmt,
-		fromDescription,
-		fullyResolvedIncrementalsDirectory,
+		from,
+		incFrom,
 		restoreStmt.Options,
 		intoDB,
 		newDBName,
-		kms)
+		kms,
+		fullyResolvedSubdir)
 	if err != nil {
 		return err
 	}