From 3e99b3b3a1a17f5f173dfb2655258ee5d8bcc986 Mon Sep 17 00:00:00 2001 From: Anne Zhu Date: Thu, 3 Jun 2021 16:31:31 -0400 Subject: [PATCH] backupccl: error message for restoring databases with AOST before GC TTL 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 --- pkg/ccl/backupccl/backup_test.go | 24 +++++++++++++++++++ pkg/ccl/backupccl/backupresolver/targets.go | 14 ++++++++--- .../backupccl/backupresolver/targets_test.go | 10 ++++---- pkg/ccl/backupccl/targets.go | 2 +- .../testdata/backup-restore/temp-tables | 2 +- 5 files changed, 42 insertions(+), 10 deletions(-) diff --git a/pkg/ccl/backupccl/backup_test.go b/pkg/ccl/backupccl/backup_test.go index 4383825bf8d4..e653d16c3900 100644 --- 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 index 53c9f5dd5b9b..71c61bc8d631 100644 --- 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 index 6e85063262d5..0e4dcd270d3f 100644 --- 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 index 1eaf34de466f..8a1c81d04c24 100644 --- 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 index 75828a5f408a..dcf2e2afebb2 100644 --- 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/'