sql/catalog: unify the descs.Collection resolution-by-ID API by thoszhang · Pull Request #57343 · cockroachdb/cockroach

thoszhang · 2020-12-02T00:04:53Z

See commits for details. This PR mostly standardizes the ByID methods on descs.Collection for the specific descriptor types.

Closes #57539.

cockroach-teamcity · 2020-12-02T00:04:59Z

This change is

thoszhang · 2020-12-24T09:21:34Z

I think as a first pass this PR does most of what we want, API-wise. There are still some loose ends: There are a few deprecated methods left on the Collection (GetMutableTableVersionByID) whose existing usages need to be replaced, and we still don't have GetImmutableObjectByName, etc., but I think it's a reasonable stopping point.

I'm open to renaming the old GetMutableTableVersionByID and GetMutableTypeVersionByID methods to names that make more sense before we mostly set this part of the interface overhaul aside.

ajwerner

Sorry this took so long. Once things start getting hidden in the reviewable UI I lose them.

Reviewed 1 of 1 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)

pkg/sql/catalog/descs/collection.go, line 899 at r1 (raw file):

		// TODO (lucy): Consider how to avoid having to generate user-facing errors
		// here. At the very least, this is inconsistent with the name resolution
		// methods, which just return internal errors from filterDescriptorState.

Can you help me better understand what this would take?

This commit unifies the implementations of all the `descs.Collection` resolution-by-ID methods, and increases their consistency in respecting lookup flags for non-public descriptors. It also adds methods with standardized names and signatures: each descriptor type now has mutable and immutable variants for resolution by ID. All the pre-existing methods still exist, but are marked for deprecation. Release note: None

This commit performs a few trivial replacements of `descs.Collection` methods with their new names: - `GetTableVersionByID` -> `GetImmutableTableByID` - `GetTypeVersionByID` -> `GetImmutableTypeByID` - `ResolveSchemaByID` -> `GetImmutableSchemaByID` (which also takes an explicit flag) - `GetDatabaseVersionByID` -> `GetImmutableDatabaseByID` All the `Required` values have also been removed from the flags passed into these methods, since they unconditionally return an error when the descriptor does not exist. Release note: None

thoszhang

Also removed the explicit Required flags from the ByID method calls, since we ignore that flag.

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

pkg/sql/catalog/descs/collection.go, line 899 at r1 (raw file):

Previously, ajwerner wrote…

		// TODO (lucy): Consider how to avoid having to generate user-facing errors
		// here. At the very least, this is inconsistent with the name resolution
		// methods, which just return internal errors from filterDescriptorState.

Can you help me better understand what this would take?

I deleted this comment, since I'm not convinced that this is a problem, or at least not the most important problem.

I just fixed a bug in this PR that caused TestInvalidOwnedDescriptorsAreDroppable to fail, due to us checking specifically for catalog.ErrDescriptorNotFound (which we used to directly return out of the physical accessor, or something, but now gets discarded in favor of the various user-facing errors in sqlerrors). I fixed it by having us check for pgcode.UndefinedTable instead, but I don't know that this is a good idea.

The point is, it seems potentially useful to have a reliable sentinel error indicating that no such descriptor exists with the given name/ID. We didn't really do this starting with the name resolution APIs, so I don't want to do it in this PR, but I opened #59027 with some thoughts.

ajwerner

cc @postamar - let's get you tagged on reviews like this.

Reviewed 1 of 3 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner)

ajwerner

Reviewed 2 of 3 files at r2, 14 of 14 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained

pkg/sql/catalog/descs/collection.go, line 899 at r1 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

I deleted this comment, since I'm not convinced that this is a problem, or at least not the most important problem.

I just fixed a bug in this PR that caused TestInvalidOwnedDescriptorsAreDroppable to fail, due to us checking specifically for catalog.ErrDescriptorNotFound (which we used to directly return out of the physical accessor, or something, but now gets discarded in favor of the various user-facing errors in sqlerrors). I fixed it by having us check for pgcode.UndefinedTable instead, but I don't know that this is a good idea.

The point is, it seems potentially useful to have a reliable sentinel error indicating that no such descriptor exists with the given name/ID. We didn't really do this starting with the name resolution APIs, so I don't want to do it in this PR, but I opened #59027 with some thoughts.

I think the answer here should be to leverage errors.Mark to make sure that the meaningful, identifiable error object propagates. Using the pgcodes doesn't make me happy.

thoszhang

TFTR!

bors r+

Reviewable status: complete! 1 of 0 LGTMs obtained

pkg/sql/catalog/descs/collection.go, line 899 at r1 (raw file):

Previously, ajwerner wrote…

I think the answer here should be to leverage errors.Mark to make sure that the meaningful, identifiable error object propagates. Using the pgcodes doesn't make me happy.

FWIW, we already check for that pgcode here, where the logic is basically the same:

cockroach/pkg/sql/drop_sequence.go

Lines 170 to 174 in 4fcc73a

    
           if errors.Is(err, catalog.ErrDescriptorDropped) || 
        
           	pgerror.GetPGCode(err) == pgcode.UndefinedTable { 
        
           	log.Eventf(ctx, "swallowing error ensuring owned sequences can be removed: %s", err.Error()) 
        
           	continue 
        
           }

So I just copied it. But I don't like it either.

We should try to fix the errors soon, though I fear that it won't be that easy since the callers probably don't actually handle errors (or what they indicate) in a very principled way and we just rely on a lot of accidental behavior. Maybe we can do that after finally mocking out the physical access dependencies like we've been meaning to, though that in itself is not necessarily going to help us do the right things.

craig · 2021-01-15T04:30:00Z

Build succeeded:

GitHub CI (Cockroach)

thoszhang force-pushed the refactor-catalog-3 branch from 1f40abf to 697a450 Compare December 2, 2020 02:01

thoszhang mentioned this pull request Dec 15, 2020

sql: found repro for "adding questionable descriptor 4294867296" #57868

Closed

thoszhang force-pushed the refactor-catalog-3 branch 6 times, most recently from a2e7277 to 54d4893 Compare December 24, 2020 00:02

thoszhang force-pushed the refactor-catalog-3 branch 6 times, most recently from d036c0c to b260531 Compare December 24, 2020 08:46

thoszhang changed the title ~~[wip] sql/catalog: unify the descs.Collection resolution-by-ID API~~ sql/catalog: unify the descs.Collection resolution-by-ID API Dec 24, 2020

thoszhang marked this pull request as ready for review December 24, 2020 09:05

thoszhang requested review from a team, ajwerner and pbardea and removed request for a team and pbardea December 24, 2020 09:05

ajwerner approved these changes Jan 14, 2021

View reviewed changes

thoszhang force-pushed the refactor-catalog-3 branch 4 times, most recently from 7c4e97c to 32c59a6 Compare January 15, 2021 01:51

Lucy Zhang added 2 commits January 14, 2021 20:57

thoszhang force-pushed the refactor-catalog-3 branch from 32c59a6 to ee72e00 Compare January 15, 2021 01:57

thoszhang mentioned this pull request Jan 15, 2021

sql/catalog: standardize errors from descs.Collection #59027

Closed

thoszhang commented Jan 15, 2021

View reviewed changes

ajwerner reviewed Jan 15, 2021

View reviewed changes

ajwerner approved these changes Jan 15, 2021

View reviewed changes

thoszhang commented Jan 15, 2021

View reviewed changes

thoszhang mentioned this pull request Jan 15, 2021

sql/catalog: rename deprecated descs.Collection methods #59029

Closed

craig bot merged commit a786c51 into cockroachdb:master Jan 15, 2021

thoszhang deleted the refactor-catalog-3 branch January 15, 2021 04:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql/catalog: unify the descs.Collection resolution-by-ID API#57343

sql/catalog: unify the descs.Collection resolution-by-ID API#57343
craig[bot] merged 2 commits intocockroachdb:masterfrom
thoszhang:refactor-catalog-3

thoszhang commented Dec 2, 2020 •

edited

Loading

Uh oh!

cockroach-teamcity commented Dec 2, 2020

Uh oh!

thoszhang commented Dec 24, 2020

Uh oh!

ajwerner left a comment

Uh oh!

thoszhang left a comment

Uh oh!

ajwerner left a comment

Uh oh!

ajwerner left a comment

Uh oh!

thoszhang left a comment

Uh oh!

craig bot commented Jan 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if errors.Is(err, catalog.ErrDescriptorDropped) \|\|
	pgerror.GetPGCode(err) == pgcode.UndefinedTable {
	log.Eventf(ctx, "swallowing error ensuring owned sequences can be removed: %s", err.Error())
	continue
	}

Conversation

thoszhang commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Dec 2, 2020

Uh oh!

thoszhang commented Dec 24, 2020

Uh oh!

ajwerner 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!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

ajwerner 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!

craig bot commented Jan 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thoszhang commented Dec 2, 2020 •

edited

Loading