-
Notifications
You must be signed in to change notification settings - Fork 4.1k
sql: stop returning raw protobufs from descriptor accessors #56306
Description
Currently, the interfaces defined for the top level descriptor types (catalog.{,Database,Schema,Table,Type}Descriptor) return raw protobufs from most of their methods. This is undesirable (at minimum) because the return values are easily mutated by accident, and because there's no opportunity to extend the raw protobufs with more advanced caching data structures.
Adding interfaces here instead of raw protos will also improve API uniformity and make our codebase more easily tested and understood.
The goal of this issue is to wrap the returned protobufs in an struct and accompanying interface that hides the underlying protobufs. Some of the leaf structs matter less - for example, UserPrivilege isn't important to wrap. It's more important to prevent the big ones from mutability: ColumnDescriptor, IndexDescriptor, and so on.
We'll be extending catalog.Descriptor and its sub-interfaces (DatabaseDescriptor, SchemaDescriptor, TableDescriptor, TypeDescriptor) so that their methods do not return things from the descpb package at all.
For example, we'll want to start returning a new catalog.IndexDescriptor interface instead of descpb.IndexDescriptor from TableDescriptor.GetPrimaryIndex().
Here is the list of interfaces we'll want to extract into sql/catalog, along with associated new structs inside of sql/catalog/tabledesc:
ColumnFamilyDescriptorIndexDescriptorsql: stop returning raw protobufs for IndexDescriptor API getters #57465ColumnDescriptorsql: stop returning raw protobufs for ColumnDescriptor API getters #59805PrivilegeDescriptor- Constraints - we need to figure out what to do here in more detail.
We'll need to figure out a pattern for iteration as well, for methods like GetPublicNonPrimaryIndexes(). We don't want to have to allocate a slice of n interfaces and structs for each of these items - for example, the ColumnDescriptors inside of an IndexDescriptor.
In addition to the new interfaces listed above we'll also want to extend and use the existing catalog interfaces:
TableDescriptorsql: use catalog.TableDescriptor instead of tabledesc.Immutable #59486DatabaseDescriptor,SchemaDescriptor,TypeDescriptorsql: use catalog.*Descriptor instead of (dbdesc|schemadesc|typedesc).Immutable #62750Column,Indexsql: use catalog.Column and catalog.Index outside of catalog #63755
We'll also want to unify the new interfaces with those in pkg/sql/opt/cat at some point:
cat.Index -> catalog.Indexcat.Column -> catalog.Column- others as well most likely (TBD)