sql/*: remove catalog.ResolvedSchema, generalize SchemaDescriptor #65142

ajwerner · 2021-05-13T17:07:22Z

The ResolvedSchema was a blemish on the descriptor resolution APIs. It made
it very hard to create a reasonable abstraction for descriptor resolution
injection. This commit creates new implementations of SchemaDescriptor to
represent virtual, public, and temporary schemas.

There is likely more cleanup that could be done along the way to make the
different kinds of schemas more uniform, but that is left for later or never.

Relates to #64089.

Release note: None

cockroach-teamcity · 2021-05-13T17:07:29Z

This change is

postamar

Nits aside all my comments can basically be boiled down to: I believe we'll be better off in the long run if you build individual instances of virtual schema descriptors based on their parent ID, and following the same logic, might as well pass the the privileges along with the parent ID when building both virtual and temp schemas.

postamar · 2021-05-19T13:09:39Z

pkg/sql/catalog/accessor.go

@@ -40,11 +40,11 @@ type Accessor interface {
 		ctx context.Context, txn *kv.Txn, dbName string, flags tree.DatabaseLookupFlags,
 	) (DatabaseDescriptor, error)

-	// GetSchema returns true and a ResolvedSchema object if the target schema
+	// GetSchema returns true and a SchemaDescriptor object if the target schema


Function name in docstring doesn't match declaration. Strange that the linter doesn't complain. I filed an issue: #65459

postamar · 2021-05-19T13:13:28Z

pkg/sql/catalog/accessor.go

 	// exists under the target database.
 	GetSchemaByName(
 		ctx context.Context, txn *kv.Txn, dbID descpb.ID, scName string, flags tree.SchemaLookupFlags,
-	) (bool, ResolvedSchema, error)
+	) (bool, SchemaDescriptor, error)


Considering that we return an interface instead of struct now, should we do away with the bool return field, since it is equivalent to a nil check?

I know there are two schools of thought on this matter. Do we have a consensus on this at CRL? If so I haven't noticed. Personally I lean slightly towards nil-checking for a bunch of debatable reasons which aren't really super interesting but honestly I don't care too much either way.

postamar · 2021-05-19T13:18:48Z

pkg/sql/catalog/schemadesc/public_schema_desc.go

@@ -0,0 +1,52 @@
+// Copyright 2020 The Cockroach Authors.


postamar · 2021-05-19T13:23:34Z

pkg/sql/catalog/schemadesc/synthetic_schema_desc.go

+	return nil
+}
+func (p synthetic) GetPrivileges() *descpb.PrivilegeDescriptor {
+	// TODO(ajwerner): Should this panic?


I think it either should implement this, or log.Fatal/panic.

postamar · 2021-05-19T13:25:08Z

pkg/sql/catalog/schemadesc/synthetic_schema_desc.go

+	return ""
+}
+func (p synthetic) DescriptorProto() *descpb.Descriptor {
+	panic(errors.AssertionFailedf("%s schema cannot be encoded", p.kindName()))


I thought we were supposed to log.Fatalf instead of panicking these days.

postamar · 2021-05-19T13:34:13Z

pkg/sql/catalog/schemadesc/temporary_schema_desc.go

+	parentID descpb.ID
+}
+
+var _ catalog.SchemaDescriptor = temporary{}


Shouldn't this implement GetPrivileges properly, at least? Perhaps it's time for a catalog.Privileges interface.

The same comment applies to virtual schemas.

postamar · 2021-05-19T13:34:24Z

pkg/sql/catalog/schemadesc/virtual_schema_desc.go

@@ -0,0 +1,64 @@
+// Copyright 2020 The Cockroach Authors.


postamar · 2021-05-19T13:41:26Z

pkg/sql/catalog/schemadesc/virtual_schema_desc.go

+// returned descriptor is not mapped to a database; every database has all of
+// the same virtual schemas and the ParentID on the returned descriptor will be
+// descpb.InvalidID.
+func GetVirtualSchemaByID(id descpb.ID) (catalog.SchemaDescriptor, bool) {


Would it be possible to pass the parent ID to GetVirtualSchemaByID and build the descriptor object with that parent ID? I feel uneasy about having the same descriptor for a whole class of schemas, somehow.

postamar · 2021-05-19T13:45:39Z

pkg/sql/information_schema.go

 					} else {
 						// Other schemas inherit from the parent database.
+						// TODO(ajwerner): Fix this because it's bogus for everything other
+						// than public.


Implementing GetPrivileges for synthetic schemas would obviate the need for branching on SchemaKind() altogether, it seems.

postamar · 2021-05-19T13:48:33Z

pkg/sql/pg_catalog.go

 						// admin is the owner of the public schema.
+						//
+						// TODO(ajwerner): The public schema effectively carries the privileges
+						// of the database so consider using the database's owner for public.


Same comment re. GetPrivileges.

ajwerner · 2021-05-20T04:28:13Z

Nits aside all my comments can basically be boiled down to: I believe we'll be better off in the long run if you build individual instances of virtual schema descriptors based on their parent ID, and following the same logic, might as well pass the the privileges

You may be entertained to learn that my initial implementation had fully independent implementations for the 3 types and I stored real privileges on the virtual schemas. I refactored it because it felt like too much copy-pasta. I'll revert that and add the parent ID to the virtual schemas.

ajwerner · 2021-05-20T04:29:44Z

Oof, misread, you're saying independent instance not implementations. Ack, that sounds good to me. No need to optimize the already allocation-wasteful virtual stuff and performance hilarious temp schemas.

ajwerner

I've been chewing on this and am effectively waving off most of your meaty comments.

I'm inclined to get this in with an agreement that we should have independent objects. I'd like to clean up the collection first in order to make it straightforward and clean to inject the various sources of these instances. I'm not keen on allocating a new public schema for each transaction, let alone for each resolution. That means I need to cache these things somewhere. I'm happy to file an issue to make sure it happens.

Same goes for the bools. We should have consistency but right now it's an inconsistent mess. I started stripping all the bools and it got big and felt worthy of its own PR.

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

pkg/sql/information_schema.go, line 1176 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

Implementing GetPrivileges for synthetic schemas would obviate the need for branching on SchemaKind() altogether, it seems.

This turns out to be annoying to do. The abstractions in the collection aren't very good. I want to refactor some other things and then come back to this if you're okay with it.

pkg/sql/catalog/accessor.go, line 43 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

Function name in docstring doesn't match declaration. Strange that the linter doesn't complain. I filed an issue: #65459

Done.

pkg/sql/catalog/accessor.go, line 47 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

Considering that we return an interface instead of struct now, should we do away with the bool return field, since it is equivalent to a nil check?

I know there are two schools of thought on this matter. Do we have a consensus on this at CRL? If so I haven't noticed. Personally I lean slightly towards nil-checking for a bunch of debatable reasons which aren't really super interesting but honestly I don't care too much either way.

I'm going to do this in follow up. It turns out that there are a lot of these to remove.

pkg/sql/catalog/schemadesc/public_schema_desc.go, line 1 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

nit: 2021

Done.

pkg/sql/catalog/schemadesc/virtual_schema_desc.go, line 26 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

Would it be possible to pass the parent ID to GetVirtualSchemaByID and build the descriptor object with that parent ID? I feel uneasy about having the same descriptor for a whole class of schemas, somehow.

I agree with you but I'd like to make a more conservative change first.

ajwerner

Removed a bunch of boolean "found" return values. Filed #65931.

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

pkg/sql/catalog/accessor.go, line 47 at r1 (raw file):

Previously, ajwerner wrote…

I'm going to do this in follow up. It turns out that there are a lot of these to remove.

I added two more commits that remove this and a lot of these in general.

postamar · 2021-06-01T19:03:56Z

I hadn't considered the impact of caching. In any case I'm OK with you not going forward with beefing up the synthetic descriptors, this change as it is is a net improvement over the existing code. That being said there remain a few comments about error handling but they're minor.

ajwerner

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar)

pkg/sql/pg_catalog.go, line 1724 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

Same comment re. GetPrivileges.

Deferred

pkg/sql/catalog/schemadesc/synthetic_schema_desc.go, line 51 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

I think it either should implement this, or log.Fatal/panic.

Done.

pkg/sql/catalog/schemadesc/synthetic_schema_desc.go, line 76 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

I thought we were supposed to log.Fatalf instead of panicking these days.

Done.

pkg/sql/catalog/schemadesc/synthetic_schema_desc.go, line 79 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

I believe this should return the parent ID in all cases.

Discussed, will occur in a later follow-up.

pkg/sql/catalog/schemadesc/synthetic_schema_desc.go, line 82 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

I think in these cases we should either do nothing at all, signifying that it's OK to validate these synthetic descriptors, or we should log.Fatal/panic, signifying that it's not OK to do so at all.

I don't see why these couldn't be validated even though they're de-facto immutable, so I lean towards doing nothing at all.

Done.

pkg/sql/catalog/schemadesc/temporary_schema_desc.go, line 43 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

The same comment applies to virtual schemas.

Deferred

pkg/sql/catalog/schemadesc/virtual_schema_desc.go, line 26 at r1 (raw file):

Previously, ajwerner wrote…

I agree with you but I'd like to make a more conservative change first.

Deferred.

The `ResolvedSchema` was a blemish on the descriptor resolution APIs. It made it very hard to create a reasonable abstraction for descriptor resolution injection. This commit creates new implementations of `SchemaDescriptor` to represent virtual, public, and temporary schemas. There is likely more cleanup that could be done along the way to make the different kinds of schemas more uniform, but that is left for later or never. Release note: None

Broadly part of clean up. Release note: None

This should be inferred from the return values. Release note: None

ajwerner · 2021-06-04T13:24:26Z

@postamar friendly ping, I'm accumulating a good number of commits on top of this that I'd like to post.

postamar · 2021-06-04T16:55:19Z

Whoops sorry! I'd actually gone through this yesterday but evidently forgot to hit the ✅ button. LGTM

ajwerner · 2021-06-04T17:04:30Z

TFTR!

bors r=postamar

craig · 2021-06-04T18:58:42Z

Build succeeded:

GitHub CI (Cockroach)

ajwerner force-pushed the ajwerner/remove-resolved-schema branch 6 times, most recently from 38e891d to c7a235e Compare May 14, 2021 21:53

ajwerner requested review from a team and postamar May 14, 2021 21:53

ajwerner marked this pull request as ready for review May 14, 2021 21:53

ajwerner requested a review from a team May 14, 2021 21:53

ajwerner requested a review from a team as a code owner May 14, 2021 21:53

ajwerner requested review from pbardea and removed request for a team May 14, 2021 21:53

pbardea removed their request for review May 17, 2021 02:31

postamar suggested changes May 19, 2021

View reviewed changes

ajwerner force-pushed the ajwerner/remove-resolved-schema branch from c7a235e to 636e44f Compare June 1, 2021 13:19

ajwerner commented Jun 1, 2021

View reviewed changes

ajwerner mentioned this pull request Jun 1, 2021

sql/catalog: remove SchemaKind from SchemaDescriptor #65931

Open

ajwerner commented Jun 1, 2021

View reviewed changes

ajwerner force-pushed the ajwerner/remove-resolved-schema branch from ce4d10e to 569aef3 Compare June 2, 2021 02:21

ajwerner commented Jun 2, 2021

View reviewed changes

ajwerner force-pushed the ajwerner/remove-resolved-schema branch 2 times, most recently from 6f8031b to ace963a Compare June 2, 2021 14:42

ajwerner added 3 commits June 2, 2021 11:48

sql: remove boolean return arg from database retrieval methods

affbbd6

Broadly part of clean up. Release note: None

sql/catalog: remove bool found parameter from schema access

55ad7eb

This should be inferred from the return values. Release note: None

ajwerner force-pushed the ajwerner/remove-resolved-schema branch from ace963a to 55ad7eb Compare June 2, 2021 15:51

postamar approved these changes Jun 4, 2021

View reviewed changes

craig bot merged commit 02830c2 into cockroachdb:master Jun 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql/*: remove catalog.ResolvedSchema, generalize SchemaDescriptor #65142

sql/*: remove catalog.ResolvedSchema, generalize SchemaDescriptor #65142

ajwerner commented May 13, 2021 •

edited

Loading

cockroach-teamcity commented May 13, 2021

postamar left a comment

postamar May 19, 2021

postamar May 19, 2021

postamar May 19, 2021

postamar May 19, 2021 •

edited

Loading

postamar May 19, 2021

postamar May 19, 2021

postamar May 19, 2021

postamar May 19, 2021

postamar May 19, 2021

postamar May 19, 2021

postamar May 19, 2021

ajwerner commented May 20, 2021

ajwerner commented May 20, 2021

ajwerner left a comment

ajwerner left a comment

postamar commented Jun 1, 2021

ajwerner left a comment

ajwerner commented Jun 4, 2021

postamar commented Jun 4, 2021

ajwerner commented Jun 4, 2021

craig bot commented Jun 4, 2021

sql/*: remove catalog.ResolvedSchema, generalize SchemaDescriptor #65142

sql/*: remove catalog.ResolvedSchema, generalize SchemaDescriptor #65142

Conversation

ajwerner commented May 13, 2021 • edited Loading

cockroach-teamcity commented May 13, 2021

postamar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

postamar May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajwerner commented May 20, 2021

ajwerner commented May 20, 2021

ajwerner left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

postamar commented Jun 1, 2021

ajwerner left a comment

Choose a reason for hiding this comment

ajwerner commented Jun 4, 2021

postamar commented Jun 4, 2021

ajwerner commented Jun 4, 2021

craig bot commented Jun 4, 2021

ajwerner commented May 13, 2021 •

edited

Loading

postamar May 19, 2021 •

edited

Loading