Merge #66025

66025: backupccl: error message for restoring databases with AOST before GC TTL r=adityamaru a=annezhu98 Previously, if a target database was not found for a RESTORE, whether it's due to AOST being before GC TTL or attempting to restore a database that has never existed, the same error message gets printed. Since the errors do not occur for the same reason, having different error messages would be more informative for users to understand the cause of error. Resolves: #47105 Release note (bug fix): more accurate error message for restoring AOST before GC TTL Co-authored-by: Anne Zhu <[email protected]>
cockroachdb · Jun 7, 2021 · 3f04bd5 · 3f04bd5
2 parents 692fa83 + 3e99b3b
commit 3f04bd5
Show file tree

Hide file tree

Showing 5 changed files with 42 additions and 10 deletions.
diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go
@@ -4120,6 +4120,30 @@ func TestRestoreAsOfSystemTimeGCBounds(t *testing.T) {
 	sqlDB.Exec(
 		t, fmt.Sprintf(`RESTORE data.bank FROM $1 AS OF SYSTEM TIME %s`, postGC), lateFullTableBackup,
 	)
+
+	t.Run("restore-pre-gc-aost", func(t *testing.T) {
+		backupPath := dir + "/tbl-before-gc"
+		_, _, sqlDB, _, cleanupFn := BackupRestoreTestSetup(t, singleNode, 0, InitManualReplication)
+		defer cleanupFn()
+
+		sqlDB.Exec(t, "CREATE DATABASE db")
+		sqlDB.Exec(t, "CREATE TABLE db.a (k int, v string)")
+		sqlDB.Exec(t, `BACKUP DATABASE db TO $1 WITH revision_history`, backupPath)
+
+		sqlDB.Exec(t, "DROP TABLE db.a")
+		sqlDB.ExpectErr(
+			t, `pq: failed to resolve targets in the BACKUP location specified by the RESTORE stmt, use SHOW BACKUP to find correct targets: table "db.a" does not exist, or invalid RESTORE timestamp: supplied backups do not cover requested time`,
+			fmt.Sprintf(`RESTORE TABLE db.a FROM $1 AS OF SYSTEM TIME %s`, preGC),
+			backupPath,
+		)
+
+		sqlDB.Exec(t, "DROP DATABASE db")
+		sqlDB.ExpectErr(
+			t, `pq: failed to resolve targets in the BACKUP location specified by the RESTORE stmt, use SHOW BACKUP to find correct targets: database "db" does not exist, or invalid RESTORE timestamp: supplied backups do not cover requested time`,
+			fmt.Sprintf(`RESTORE DATABASE db FROM $1 AS OF SYSTEM TIME %s`, preGC),
+			backupPath,
+		)
+	})
 }
 
 func TestAsOfSystemTimeOnRestoredData(t *testing.T) {

diff --git a/pkg/ccl/backupccl/backupresolver/targets.go b/pkg/ccl/backupccl/backupresolver/targets.go
@@ -248,6 +248,7 @@ func DescriptorsMatchingTargets(
 	searchPath sessiondata.SearchPath,
 	descriptors []catalog.Descriptor,
 	targets tree.TargetList,
+	asOf hlc.Timestamp,
 ) (DescriptorsMatched, error) {
 	ret := DescriptorsMatched{}
 
@@ -258,11 +259,15 @@ func DescriptorsMatchingTargets(
 
 	alreadyRequestedDBs := make(map[descpb.ID]struct{})
 	alreadyExpandedDBs := make(map[descpb.ID]struct{})
+	invalidRestoreTsErr := errors.Errorf("supplied backups do not cover requested time")
 	// Process all the DATABASE requests.
 	for _, d := range targets.Databases {
 		dbID, ok := resolver.DbsByName[string(d)]
 		if !ok {
-			return ret, errors.Errorf("unknown database %q", d)
+			if asOf.IsEmpty() {
+				return ret, errors.Errorf("database %q does not exist", d)
+			}
+			return ret, errors.Wrapf(invalidRestoreTsErr, "database %q does not exist, or invalid RESTORE timestamp", d)
 		}
 		if _, ok := alreadyRequestedDBs[dbID]; !ok {
 			desc := resolver.DescByID[dbID]
@@ -355,7 +360,10 @@ func DescriptorsMatchingTargets(
 			p.ObjectNamePrefix = prefix
 			doesNotExistErr := errors.Errorf(`table %q does not exist`, tree.ErrString(p))
 			if !found {
-				return ret, doesNotExistErr
+				if asOf.IsEmpty() {
+					return ret, doesNotExistErr
+				}
+				return ret, errors.Wrapf(invalidRestoreTsErr, `table %q does not exist, or invalid RESTORE timestamp`, tree.ErrString(p))
 			}
 			tableDesc, isTable := descI.(catalog.TableDescriptor)
 			// If the type assertion didn't work, then we resolved a type instead, so
@@ -513,7 +521,7 @@ func ResolveTargetsToDescriptors(
 
 	var matched DescriptorsMatched
 	if matched, err = DescriptorsMatchingTargets(ctx,
-		p.CurrentDatabase(), p.CurrentSearchPath(), allDescs, *targets); err != nil {
+		p.CurrentDatabase(), p.CurrentSearchPath(), allDescs, *targets, endTime); err != nil {
 		return nil, nil, err
 	}
 

diff --git a/pkg/ccl/backupccl/backupresolver/targets_test.go b/pkg/ccl/backupccl/backupresolver/targets_test.go
@@ -171,16 +171,16 @@ func TestDescriptorsMatchingTargets(t *testing.T) {
 		err             string
 	}{
 		{"", "DATABASE system", []string{"system", "foo", "bar"}, []string{"system"}, ``},
-		{"", "DATABASE system, noexist", nil, nil, `unknown database "noexist"`},
+		{"", "DATABASE system, noexist", nil, nil, `database "noexist" does not exist`},
 		{"", "DATABASE system, system", []string{"system", "foo", "bar"}, []string{"system"}, ``},
 		{"", "DATABASE data", []string{"data", "baz"}, []string{"data"}, ``},
 		{"", "DATABASE system, data", []string{"system", "foo", "bar", "data", "baz"}, []string{"data", "system"}, ``},
-		{"", "DATABASE system, data, noexist", nil, nil, `unknown database "noexist"`},
+		{"", "DATABASE system, data, noexist", nil, nil, `database "noexist" does not exist`},
 		{"system", "DATABASE system", []string{"system", "foo", "bar"}, []string{"system"}, ``},
-		{"system", "DATABASE system, noexist", nil, nil, `unknown database "noexist"`},
+		{"system", "DATABASE system, noexist", nil, nil, `database "noexist" does not exist`},
 		{"system", "DATABASE data", []string{"data", "baz"}, []string{"data"}, ``},
 		{"system", "DATABASE system, data", []string{"system", "foo", "bar", "data", "baz"}, []string{"data", "system"}, ``},
-		{"system", "DATABASE system, data, noexist", nil, nil, `unknown database "noexist"`},
+		{"system", "DATABASE system, data, noexist", nil, nil, `database "noexist" does not exist`},
 
 		{"", "TABLE foo", nil, nil, `table "foo" does not exist`},
 		{"system", "TABLE foo", []string{"system", "foo"}, nil, ``},
@@ -256,7 +256,7 @@ func TestDescriptorsMatchingTargets(t *testing.T) {
 			targets := stmt.AST.(*tree.Grant).Targets
 
 			matched, err := DescriptorsMatchingTargets(context.Background(),
-				test.sessionDatabase, searchPath, descriptors, targets)
+				test.sessionDatabase, searchPath, descriptors, targets, hlc.Timestamp{})
 			if test.err != "" {
 				if !testutils.IsError(err, test.err) {
 					t.Fatalf("expected error matching '%v', but got '%v'", test.err, err)

diff --git a/pkg/ccl/backupccl/targets.go b/pkg/ccl/backupccl/targets.go
@@ -468,7 +468,7 @@ func selectTargets(
 	}
 
 	matched, err := backupresolver.DescriptorsMatchingTargets(ctx,
-		p.CurrentDatabase(), p.CurrentSearchPath(), allDescs, targets)
+		p.CurrentDatabase(), p.CurrentSearchPath(), allDescs, targets, asOf)
 	if err != nil {
 		return nil, nil, nil, err
 	}

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/temp-tables b/pkg/ccl/backupccl/testdata/backup-restore/temp-tables
@@ -35,7 +35,7 @@ public
 exec-sql
 BACKUP TABLE temp_table TO 'nodelocal://0/temp_table_backup'
 ----
-pq: failed to resolve targets specified in the BACKUP stmt: table "temp_table" does not exist
+pq: failed to resolve targets specified in the BACKUP stmt: table "temp_table" does not exist, or invalid RESTORE timestamp: supplied backups do not cover requested time
 
 exec-sql
 BACKUP DATABASE d1 TO 'nodelocal://0/d1_backup/'