backupccl: fix crash in SHOW BACKUP when types have cross-db references by pbardea · Pull Request #55774 · cockroachdb/cockroach

pbardea · 2020-10-20T19:47:53Z

Previously, SHOW BACKUP would expect that the database and schema it
references is in the backup. In the case that we had a cross-db
reference, the schema or database that it references would not be
included in the backup, and would crash. It now reports objects that it
cannot find as NULL.

Fixes #55772.

Release note (bug fix): Fix crash when performing a SHOW BACKUP against
a backup which contains a table that references a type in another
database. This state was only reachable in 20.2 release candidates and
earlier.

cockroach-teamcity · 2020-10-20T19:48:01Z

This change is

pbardea · 2020-10-20T20:18:08Z

cc @ajwerner @lucy-zhang

This is a first attempt at it. It's getting close, but I'm running into issues with hydrating the descriptors in this cross-db reference state...

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x5362921]

goroutine 3542 [running]:
github.com/cockroachdb/cockroach/pkg/sql/catalog/descs.HydrateGivenDescriptors.func1(0x93e8900, 0xc0024f8bd0, 0x36, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        /Users/pbardea/Developer/go/src/github.com/cockroachdb/cockroach/pkg/sql/catalog/descs/collection.go:1601 +0xd1
github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc.TypeLookupFunc.GetTypeDescriptor(0xc0016135e0, 0x93e8900, 0xc0024f8bd0, 0x36, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        /Users/pbardea/Developer/go/src/github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc/type_desc.go:521 +0x87
github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc.HydrateTypesInTableDescriptor.func1(0xc0005dcc00, 0xc001b2b638, 0x4011048)
        /Users/pbardea/Developer/go/src/github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc/type_desc.go:555 +0x123
github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc.HydrateTypesInTableDescriptor(0x93e8900, 0xc0024f8bd0, 0xc001938000, 0x935ec40, 0xc0016135e0, 0x3, 0xc00223b748)
        /Users/pbardea/Developer/go/src/github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc/type_desc.go:567 +0xd1
github.com/cockroachdb/cockroach/pkg/sql/catalog/descs.HydrateGivenDescriptors(0x93e8900, 0xc0024f8bd0, 0xc001a1ebc0, 0x4, 0x4, 0x0, 0x0)
        /Users/pbardea/Developer/go/src/github.com/cockroachdb/cockroach/pkg/sql/catalog/descs/collection.go:1612 +0x1ee
github.com/cockroachdb/cockroach/pkg/sql.newInternalLookupCtxFromDescriptors(0x93e8900, 0xc0024f8bd0, 0xc001a1eb40, 0x4, 0x4, 0x0, 0x0, 0x0, 0xbb3bd10)
        /Users/pbardea/Developer/go/src/github.com/cockroachdb/cockroach/pkg/sql/resolver.go:709 +0x9bf
github.com/cockroachdb/cockroach/pkg/sql.(*planner).ShowCreate(0xc001856418, 0x93e8900, 0xc0024f8bd0, 0xc00223b748, 0x7, 0xc001a1eb40, 0x4, 0x4, 0xc002eab900, 0x3, ...)
        /Users/pbardea/Developer/go/src/github.com/cockroachdb/cockroach/pkg/sql/show_create.go:245 +0x163
github.com/cockroachdb/cockroach/pkg/ccl/backupccl.backupShowerDefault.func1(0xc001d40dc0, 0x1, 0x1, 0x1, 0x1, 0x1, 0x0, 0x0)
        /Users/pbardea/Developer/go/src/github.com/cockroachdb/cockroach/pkg/ccl/backupccl/show.go:328 +0x1542
github.com/cockroachdb/cockroach/pkg/ccl/backupccl.showBackupPlanHook.func1(0x93e8840, 0xc0024f0400, 0x0, 0x0, 0x0, 0xc001f7d320, 0x0, 0x0)
        /Users/pbardea/Developer/go/src/github.com/cockroachdb/cockroach/pkg/ccl/backupccl/show.go:186 +0x80e
github.com/cockroachdb/cockroach/pkg/sql.(*hookFnNode).startExec.func1(0xc0017cd580, 0x93e8840, 0xc0024f0400, 0xc00176bb80, 0xc001856418)
        /Users/pbardea/Developer/go/src/github.com/cockroachdb/cockroach/pkg/sql/planhook.go:146 +0x7d
created by github.com/cockroachdb/cockroach/pkg/sql.(*hookFnNode).startExec
        /Users/pbardea/Developer/go/src/github.com/cockroachdb/cockroach/pkg/sql/planhook.go:145 +0xcc
FAIL    github.com/cockroachdb/cockroach/pkg/ccl/backupccl      3.194s

ajwerner · 2020-10-20T20:31:24Z

Try:

diff --git a/pkg/sql/catalog/descs/collection.go b/pkg/sql/catalog/descs/collection.go
index 1985efab32..ac8f123602 100644
--- a/pkg/sql/catalog/descs/collection.go
+++ b/pkg/sql/catalog/descs/collection.go
@@ -1585,8 +1585,17 @@ func HydrateGivenDescriptors(ctx context.Context, descs []catalog.Descriptor) er
                // we need to hydrate our types. Set up an accessor for the type hydration
                // method to look into the scanned set of descriptors.
                typeLookup := func(ctx context.Context, id descpb.ID) (tree.TypeName, catalog.TypeDescriptor, error) {
-                       typDesc := typDescs[id]
-                       dbDesc := dbDescs[typDesc.ParentID]
+                       typDesc, ok := typDescs[id]
+                       if !ok {
+                               n := tree.MakeUnresolvedName(fmt.Sprintf("[%d]", id))
+                               return tree.TypeName{}, nil, sqlerrors.NewUndefinedObjectError(&n,
+                                       tree.TypeObject)
+                       }
+                       dbDesc, ok := dbDescs[typDesc.ParentID]
+                       if !ok {
+                               n := fmt.Sprintf("[%d]", typDesc.ParentID)
+                               return tree.TypeName{}, nil, sqlerrors.NewUndefinedDatabaseError(n)
+                       }

ajwerner · 2020-10-20T20:32:26Z

Maybe that is bad because it now just omits the table altogether.

ajwerner · 2020-10-20T20:33:33Z

What's the right UX here?

pbardea · 2020-10-20T20:56:25Z

Latest change logs and silently ignores any error we get showing the create statement. I also played around with displaying an error message in that cell rather than "NULL"... but I'm not sure that's the right way to go here.

ajwerner · 2020-10-20T21:06:47Z

Silently ignoring the error is sort of good, right, because it leads to omitting the table, right? That seems sane because you can't restore that table.

pbardea · 2020-10-20T21:18:22Z

We can silently ignore it in 2 ways:

Show the table in the SHOW BACKUP command (what it does now, but show the parent db and schema as NULL). This leads to a bit of confusion since we list a table that we can't restore. However, we also show that the database doesn't resovle (NULL) which is the reason we can't restore it. I also like that it keeps the simplicity of SHOW BACKUP to show me all the descriptors in the backup.
We could omit the table from the output. I don't think we should do this since SHOW BACKUP is no longer "show me all the descriptors in the backup" but rather "show me everything I can restore from the backup", which is a lot trickier. I'm not sure if just skipping over rows where the references can't be resolved is enough to be able to claim that we can restore the tables.

ajwerner · 2020-10-20T21:19:25Z

SGTM

pbardea · 2020-10-20T21:20:05Z

Reworking the tests since this will fail once we disallow the rename.

ajwerner

Reviewed 2 of 7 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @dt, @lucy-zhang, and @pbardea)

pkg/ccl/backupccl/restore_old_versions_test.go, line 89 at r1 (raw file):

					CREATE TYPE t AS ENUM ('foo');
				  CREATE TABLE tbl (a t);

(I don't care enough for you to fix it tbh) nit: spacing

pbardea · 2020-10-20T23:23:27Z

Removed the map changes so that we have the minimal set of changes go in the backport.

thoszhang

Reviewed 7 of 7 files at r1, 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dt and @pbardea)

pkg/ccl/backupccl/restore_old_versions_test.go, line 94 at r1 (raw file):

				  BACKUP DATABASE otherdb TO 'nodelocal://1/xDbRef';

					This was permitted on some release candidates of v20.2.

Add an issue number?

pkg/ccl/backupccl/restore_old_versions_test.go, line 123 at r1 (raw file):

			sqlDB.ExpectErr(t, `type "t" has unknown ParentID 50`, `RESTORE DATABASE otherdb FROM $1`, LocalFoo)

			sqlDB.CheckQueryResults(t, `

Maybe briefly comment on what we're looking for here? (i.e., no crash, and we get some NULLs)

thoszhang

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dt and @pbardea)

pkg/ccl/backupccl/restore_old_versions_test.go, line 94 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Add an issue number?

(#55709)

Previously, SHOW BACKUP would expect that the database and schema it references is in the backup. In the case that we had a cross-db reference, the schema or database that it references would not be included in the backup, and would crash. It now reports objects that it cannot find as NULL. Release note (bug fix): Fix crash when performing a SHOW BACKUP against a backup which contains a table that references a type in another database. This state was only reachable in 20.2 release candidates and earlier.

pbardea

Thanks for the reviews!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @dt, and @lucy-zhang)

pkg/ccl/backupccl/restore_old_versions_test.go, line 89 at r1 (raw file):

Previously, ajwerner wrote…

(I don't care enough for you to fix it tbh) nit: spacing

Done.

pkg/ccl/backupccl/restore_old_versions_test.go, line 94 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

(#55709)

Done.

pkg/ccl/backupccl/restore_old_versions_test.go, line 123 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Maybe briefly comment on what we're looking for here? (i.e., no crash, and we get some NULLs)

Done.

pbardea · 2020-10-21T00:41:41Z

bors r=lucy-zhang,ajwerner

craig · 2020-10-21T01:41:27Z

Build succeeded:

GitHub CI (Cockroach)

pbardea force-pushed the show-backup-x-db branch from 5962382 to 266c5b9 Compare October 20, 2020 20:15

pbardea force-pushed the show-backup-x-db branch 2 times, most recently from 2e39da5 to 84aa9f6 Compare October 20, 2020 20:54

pbardea force-pushed the show-backup-x-db branch from 84aa9f6 to b068106 Compare October 20, 2020 21:57

pbardea requested review from ajwerner and dt October 20, 2020 21:58

pbardea marked this pull request as ready for review October 20, 2020 21:58

pbardea requested a review from a team October 20, 2020 21:58

pbardea added the backport-20.2.x label Oct 20, 2020

pbardea requested a review from thoszhang October 20, 2020 22:02

pbardea force-pushed the show-backup-x-db branch from b068106 to ac74d08 Compare October 20, 2020 22:15

pbardea mentioned this pull request Oct 20, 2020

release-20.2: backupccl: fix crash in SHOW BACKUP when types have cross-db references #55786

Merged

pbardea force-pushed the show-backup-x-db branch from ac74d08 to 58bd663 Compare October 20, 2020 22:16

ajwerner approved these changes Oct 20, 2020

View reviewed changes

pbardea force-pushed the show-backup-x-db branch from 58bd663 to fa844ac Compare October 20, 2020 23:20

thoszhang approved these changes Oct 20, 2020

View reviewed changes

thoszhang reviewed Oct 20, 2020

View reviewed changes

pbardea force-pushed the show-backup-x-db branch from fa844ac to 0b6544f Compare October 20, 2020 23:42

pbardea commented Oct 21, 2020

View reviewed changes

craig bot merged commit 738848e into cockroachdb:master Oct 21, 2020

pbardea removed the backport-20.2.x label Oct 27, 2020

Conversation

pbardea commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Oct 20, 2020

Uh oh!

pbardea commented Oct 20, 2020

Uh oh!

ajwerner commented Oct 20, 2020

Uh oh!

ajwerner commented Oct 20, 2020

Uh oh!

ajwerner commented Oct 20, 2020

Uh oh!

pbardea commented Oct 20, 2020

Uh oh!

ajwerner commented Oct 20, 2020

Uh oh!

pbardea commented Oct 20, 2020

Uh oh!

ajwerner commented Oct 20, 2020

Uh oh!

pbardea commented Oct 20, 2020

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

pbardea commented Oct 20, 2020

Uh oh!

thoszhang left a comment

Choose a reason for hiding this comment

Uh oh!

thoszhang left a comment

Choose a reason for hiding this comment

Uh oh!

pbardea left a comment

Choose a reason for hiding this comment

Uh oh!

pbardea commented Oct 21, 2020

Uh oh!

craig bot commented Oct 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pbardea commented Oct 20, 2020 •

edited

Loading