From 94642ea2ca7df6c54247efbc7446288b0faf511e Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Tue, 13 Dec 2022 15:02:08 -0500 Subject: [PATCH] descs: refactor GetAll* methods This commit refactors the descs.Collection's GetAll* methods, which return descriptors and other catalog metadata in bulk, to share a common access path in which the descriptor's layers are properly aggregated. This is non-trivial as there are numerous edge cases: - virtual schemas and objects exist for all databases and their descriptors don't reference the parent database at all, - descriptorless public schemas and temporary schemas only exist as namespace entries, - functions don't have namespace entries, - and so forth. These new methods are delegated to by the existing methods which are now deprecated. Informs #64089. Release note: None --- .../testdata/benchmark_expectations | 44 +- pkg/sql/catalog/descs/collection.go | 504 ++++++++++++------ pkg/sql/catalog/descs/collection_test.go | 4 + pkg/sql/catalog/descs/uncommitted_metadata.go | 13 + pkg/sql/catalog/descs/virtual_descriptors.go | 39 +- .../internal/catkv/system_database_cache.go | 2 +- pkg/sql/catalog/tabledesc/table_desc.go | 4 +- 7 files changed, 405 insertions(+), 205 deletions(-) diff --git a/pkg/bench/rttanalysis/testdata/benchmark_expectations b/pkg/bench/rttanalysis/testdata/benchmark_expectations index 8f6a1e46e4eb..1bd78e78bbbb 100644 --- a/pkg/bench/rttanalysis/testdata/benchmark_expectations +++ b/pkg/bench/rttanalysis/testdata/benchmark_expectations @@ -33,29 +33,29 @@ exp,benchmark 14,CreateRole/create_role_with_no_options 10,DropDatabase/drop_database_0_tables 11,DropDatabase/drop_database_1_table -12,DropDatabase/drop_database_2_tables -13,DropDatabase/drop_database_3_tables +11,DropDatabase/drop_database_2_tables +11,DropDatabase/drop_database_3_tables 18,DropRole/drop_1_role 26,DropRole/drop_2_roles 34,DropRole/drop_3_roles 11,DropSequence/drop_1_sequence 12,DropSequence/drop_2_sequences -13,DropSequence/drop_3_sequences +12,DropSequence/drop_3_sequences 11,DropTable/drop_1_table 12,DropTable/drop_2_tables -13,DropTable/drop_3_tables +12,DropTable/drop_3_tables 12,DropView/drop_1_view 13,DropView/drop_2_views 13,DropView/drop_3_views 10,Grant/grant_all_on_1_table -11,Grant/grant_all_on_2_tables -11,Grant/grant_all_on_3_tables +10,Grant/grant_all_on_2_tables +10,Grant/grant_all_on_3_tables 11,GrantRole/grant_1_role 15,GrantRole/grant_2_roles -3,ORMQueries/activerecord_type_introspection_query -6,ORMQueries/django_table_introspection_1_table -6,ORMQueries/django_table_introspection_4_tables -6,ORMQueries/django_table_introspection_8_tables +4,ORMQueries/activerecord_type_introspection_query +8,ORMQueries/django_table_introspection_1_table +8,ORMQueries/django_table_introspection_4_tables +8,ORMQueries/django_table_introspection_8_tables 2,ORMQueries/has_column_privilege_using_attnum 2,ORMQueries/has_column_privilege_using_column_name 1,ORMQueries/has_schema_privilege_1 @@ -69,19 +69,19 @@ exp,benchmark 6,ORMQueries/has_table_privilege_5 85,ORMQueries/hasura_column_descriptions 85,ORMQueries/hasura_column_descriptions_8_tables -6,ORMQueries/hasura_column_descriptions_modified -4,ORMQueries/information_schema._pg_index_position -3,ORMQueries/pg_attribute -3,ORMQueries/pg_class -7,ORMQueries/pg_is_other_temp_schema -7,ORMQueries/pg_is_other_temp_schema_multiple_times -4,ORMQueries/pg_my_temp_schema -4,ORMQueries/pg_my_temp_schema_multiple_times -4,ORMQueries/pg_namespace -3,ORMQueries/pg_type +8,ORMQueries/hasura_column_descriptions_modified +6,ORMQueries/information_schema._pg_index_position +5,ORMQueries/pg_attribute +5,ORMQueries/pg_class +9,ORMQueries/pg_is_other_temp_schema +9,ORMQueries/pg_is_other_temp_schema_multiple_times +6,ORMQueries/pg_my_temp_schema +6,ORMQueries/pg_my_temp_schema_multiple_times +6,ORMQueries/pg_namespace +5,ORMQueries/pg_type 10,Revoke/revoke_all_on_1_table -11,Revoke/revoke_all_on_2_tables -11,Revoke/revoke_all_on_3_tables +10,Revoke/revoke_all_on_2_tables +10,Revoke/revoke_all_on_3_tables 9,RevokeRole/revoke_1_role 11,RevokeRole/revoke_2_roles 1,SystemDatabaseQueries/select_system.users_with_empty_database_Name diff --git a/pkg/sql/catalog/descs/collection.go b/pkg/sql/catalog/descs/collection.go index 69ce7e8c700a..e436461455be 100644 --- a/pkg/sql/catalog/descs/collection.go +++ b/pkg/sql/catalog/descs/collection.go @@ -30,11 +30,11 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/internal/validate" "github.com/cockroachdb/cockroach/pkg/sql/catalog/lease" "github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" "github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants" "github.com/cockroachdb/cockroach/pkg/sql/sem/catid" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" - "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/sql/sqlliveness" "github.com/cockroachdb/cockroach/pkg/sql/sqlutil" "github.com/cockroachdb/cockroach/pkg/util/hlc" @@ -696,274 +696,438 @@ func newMutableSyntheticDescriptorAssertionError(id descpb.ID) error { return errors.AssertionFailedf("attempted mutable access of synthetic descriptor %d", id) } -// GetAllDescriptorsForDatabase retrieves the complete set of descriptors -// in the requested database. -func (tc *Collection) GetAllDescriptorsForDatabase( - ctx context.Context, txn *kv.Txn, db catalog.DatabaseDescriptor, -) (nstree.Catalog, error) { - read, err := tc.cr.ScanNamespaceForDatabaseSchemasAndObjects(ctx, txn, db) +// GetAll returns all descriptors, namespace entries, comments and +// zone configs visible by the transaction. +func (tc *Collection) GetAll(ctx context.Context, txn *kv.Txn) (nstree.Catalog, error) { + stored, err := tc.cr.ScanAll(ctx, txn) + if err != nil { + return nstree.Catalog{}, err + } + ret, err := tc.aggregateAllLayers(ctx, txn, stored) + if err != nil { + return nstree.Catalog{}, err + } + return ret.Catalog, nil +} + +// GetAllDatabases is like GetAll but filtered to non-dropped databases. +func (tc *Collection) GetAllDatabases(ctx context.Context, txn *kv.Txn) (nstree.Catalog, error) { + stored, err := tc.cr.ScanNamespaceForDatabases(ctx, txn) + if err != nil { + return nstree.Catalog{}, err + } + ret, err := tc.aggregateAllLayers(ctx, txn, stored) if err != nil { return nstree.Catalog{}, err } - var ids catalog.DescriptorIDSet - ids.Add(db.GetID()) - _ = read.ForEachNamespaceEntry(func(e nstree.NamespaceEntry) error { - if e.GetParentID() != db.GetID() || tc.isShadowedName(e) { + var dbIDs catalog.DescriptorIDSet + _ = ret.ForEachDescriptor(func(desc catalog.Descriptor) error { + if desc.DescriptorType() != catalog.Database { return nil } - if !strings.HasPrefix(e.GetName(), catconstants.PgTempSchemaName) && - e.GetID() != catconstants.PublicSchemaID { - ids.Add(e.GetID()) - } + dbIDs.Add(desc.GetID()) return nil }) - for _, iterator := range []func(func(catalog.Descriptor) error) error{ - tc.uncommitted.iterateUncommittedByID, - tc.synthetic.iterateSyntheticByID, - } { - _ = iterator(func(desc catalog.Descriptor) error { - if desc.GetParentID() == db.GetID() { - ids.Add(desc.GetID()) - } - return nil - }) + return ret.FilterByIDs(dbIDs.Ordered()), nil +} + +// GetAllSchemasInDatabase is like GetAll but filtered to the schemas with +// the specified parent database. Includes virtual schemas. +func (tc *Collection) GetAllSchemasInDatabase( + ctx context.Context, txn *kv.Txn, db catalog.DatabaseDescriptor, +) (nstree.Catalog, error) { + stored, err := tc.cr.ScanNamespaceForDatabaseSchemas(ctx, txn, db) + if err != nil { + return nstree.Catalog{}, err } - // Look up all non-function descriptors. - flags := tree.CommonLookupFlags{ - AvoidLeased: true, - IncludeOffline: true, - IncludeDropped: true, + var ret nstree.MutableCatalog + if db.HasPublicSchemaWithDescriptor() { + ret, err = tc.aggregateAllLayers(ctx, txn, stored) + } else { + ret, err = tc.aggregateAllLayers(ctx, txn, stored, schemadesc.GetPublicSchema()) } - descs, err := tc.getDescriptorsByID(ctx, txn, flags, ids.Ordered()...) if err != nil { return nstree.Catalog{}, err } - var ret nstree.MutableCatalog - var functionIDs catalog.DescriptorIDSet - for _, desc := range descs { - ret.UpsertDescriptor(desc) - if sc, ok := desc.(catalog.SchemaDescriptor); ok { - _ = sc.ForEachFunctionOverload(func(overload descpb.SchemaDescriptor_FunctionOverload) error { - functionIDs.Add(overload.ID) + var schemaIDs catalog.DescriptorIDSet + _ = ret.ForEachDescriptor(func(desc catalog.Descriptor) error { + sc, ok := desc.(catalog.SchemaDescriptor) + if !ok { + return nil + } + switch sc.SchemaKind() { + case catalog.SchemaTemporary, catalog.SchemaUserDefined: + if sc.GetParentID() != db.GetID() { return nil - }) + } + } + schemaIDs.Add(desc.GetID()) + return nil + }) + return ret.FilterByIDs(schemaIDs.Ordered()), nil +} + +// GetAllObjectsInSchema is like GetAll but filtered to the objects with +// the specified parent schema. Includes virtual objects. Does not include +// dropped objects. +func (tc *Collection) GetAllObjectsInSchema( + ctx context.Context, txn *kv.Txn, db catalog.DatabaseDescriptor, sc catalog.SchemaDescriptor, +) (nstree.Catalog, error) { + var ret nstree.MutableCatalog + if sc.SchemaKind() == catalog.SchemaVirtual { + tc.virtual.addAllToCatalog(ret) + } else { + stored, err := tc.cr.ScanNamespaceForSchemaObjects(ctx, txn, db, sc) + if err != nil { + return nstree.Catalog{}, err + } + ret, err = tc.aggregateAllLayers(ctx, txn, stored, sc) + if err != nil { + return nstree.Catalog{}, err } } - // Look up all function descriptors. - descs, err = tc.getDescriptorsByID(ctx, txn, flags, functionIDs.Ordered()...) + var objectIDs catalog.DescriptorIDSet + _ = ret.ForEachDescriptor(func(desc catalog.Descriptor) error { + if desc.GetParentSchemaID() == sc.GetID() { + objectIDs.Add(desc.GetID()) + } + return nil + }) + return ret.FilterByIDs(objectIDs.Ordered()), nil +} + +// GetAllInDatabase is like the union of GetAllSchemasInDatabase and +// GetAllObjectsInSchema applied to each of those schemas. +func (tc *Collection) GetAllInDatabase( + ctx context.Context, txn *kv.Txn, db catalog.DatabaseDescriptor, +) (nstree.Catalog, error) { + stored, err := tc.cr.ScanNamespaceForDatabaseSchemasAndObjects(ctx, txn, db) if err != nil { return nstree.Catalog{}, err } - for _, desc := range descs { - ret.UpsertDescriptor(desc) + schemas, err := tc.GetAllSchemasInDatabase(ctx, txn, db) + if err != nil { + return nstree.Catalog{}, err } - return ret.Catalog, nil + var schemasSlice []catalog.SchemaDescriptor + if err := schemas.ForEachDescriptor(func(desc catalog.Descriptor) error { + sc, err := catalog.AsSchemaDescriptor(desc) + schemasSlice = append(schemasSlice, sc) + return err + }); err != nil { + return nstree.Catalog{}, err + } + ret, err := tc.aggregateAllLayers(ctx, txn, stored, schemasSlice...) + if err != nil { + return nstree.Catalog{}, err + } + var inDatabaseIDs catalog.DescriptorIDSet + _ = ret.ForEachDescriptor(func(desc catalog.Descriptor) error { + if desc.DescriptorType() == catalog.Schema { + if dbID := desc.GetParentID(); dbID != descpb.InvalidID && dbID != db.GetID() { + return nil + } + } else { + if schemas.LookupDescriptor(desc.GetParentSchemaID()) == nil { + return nil + } + } + inDatabaseIDs.Add(desc.GetID()) + return nil + }) + return ret.FilterByIDs(inDatabaseIDs.Ordered()), nil } -// GetAllDescriptors returns all descriptors visible by the transaction, -func (tc *Collection) GetAllDescriptors(ctx context.Context, txn *kv.Txn) (nstree.Catalog, error) { - read, err := tc.cr.ScanAll(ctx, txn) +// GetAllTablesInDatabase is like GetAllInDatabase but filtered to tables. +func (tc *Collection) GetAllTablesInDatabase( + ctx context.Context, txn *kv.Txn, db catalog.DatabaseDescriptor, +) (nstree.Catalog, error) { + stored, err := tc.cr.ScanNamespaceForDatabaseSchemasAndObjects(ctx, txn, db) + if err != nil { + return nstree.Catalog{}, err + } + var ret nstree.MutableCatalog + if db.HasPublicSchemaWithDescriptor() { + ret, err = tc.aggregateAllLayers(ctx, txn, stored) + } else { + ret, err = tc.aggregateAllLayers(ctx, txn, stored, schemadesc.GetPublicSchema()) + } if err != nil { return nstree.Catalog{}, err } - var ids catalog.DescriptorIDSet - _ = read.ForEachDescriptor(func(desc catalog.Descriptor) error { - ids.Add(desc.GetID()) + var inDatabaseIDs catalog.DescriptorIDSet + _ = ret.ForEachDescriptor(func(desc catalog.Descriptor) error { + if desc.DescriptorType() != catalog.Table { + return nil + } + if dbID := desc.GetParentID(); dbID != descpb.InvalidID && dbID != db.GetID() { + return nil + } + inDatabaseIDs.Add(desc.GetID()) return nil }) + return ret.FilterByIDs(inDatabaseIDs.Ordered()), nil +} + +// aggregateAllLayers is the helper function used by GetAll* methods which +// takes care to stack all of the Collection's layer appropriately and ensures +// that the returned descriptors are properly hydrated and validated. +func (tc *Collection) aggregateAllLayers( + ctx context.Context, txn *kv.Txn, stored nstree.Catalog, schemas ...catalog.SchemaDescriptor, +) (ret nstree.MutableCatalog, _ error) { + // Descriptors need to be re-read to ensure proper validation hydration etc. + // We collect their IDs for this purpose and we'll re-add them later. + var descIDs catalog.DescriptorIDSet + // Start with the known function descriptor IDs. + for _, sc := range schemas { + if sc.SchemaKind() == catalog.SchemaPublic { + // This is needed at least for the temp system db during restores. + descIDs.Add(sc.GetID()) + } + _ = sc.ForEachFunctionOverload(func(overload descpb.SchemaDescriptor_FunctionOverload) error { + descIDs.Add(overload.ID) + return nil + }) + } + // Add IDs from descriptors retrieved from the storage layer. + _ = stored.ForEachDescriptor(func(desc catalog.Descriptor) error { + descIDs.Add(desc.GetID()) + if sc, ok := desc.(catalog.SchemaDescriptor); ok { + _ = sc.ForEachFunctionOverload(func(overload descpb.SchemaDescriptor_FunctionOverload) error { + descIDs.Add(overload.ID) + return nil + }) + } + return nil + }) + // Add stored namespace entries which are not shadowed. + _ = stored.ForEachNamespaceEntry(func(e nstree.NamespaceEntry) error { + if tc.isShadowedName(e) { + return nil + } + // Temporary schemas don't have descriptors and are persisted only + // as namespace table entries. + if e.GetParentID() != descpb.InvalidID && e.GetParentSchemaID() == descpb.InvalidID && + strings.HasPrefix(e.GetName(), catconstants.PgTempSchemaName) { + ret.UpsertDescriptor(schemadesc.NewTemporarySchema(e.GetName(), e.GetID(), e.GetParentID())) + } else { + descIDs.Add(e.GetID()) + } + ret.UpsertNamespaceEntry(e, e.GetID(), e.GetMVCCTimestamp()) + return nil + }) + // Add stored comments which are not shadowed. + _ = stored.ForEachComment(func(key catalogkeys.CommentKey, cmt string) error { + if _, _, isShadowed := tc.uncommittedComments.getUncommitted(key); !isShadowed { + ret.UpsertComment(key, cmt) + } + return nil + }) + // Add stored zone configs which are not shadowed. + _ = stored.ForEachZoneConfig(func(id descpb.ID, zc catalog.ZoneConfig) error { + if _, isShadowed := tc.uncommittedZoneConfigs.getUncommitted(id); !isShadowed { + ret.UpsertZoneConfig(id, zc.ZoneConfigProto(), zc.GetRawBytesInStorage()) + } + return nil + }) + // Add uncommitted and synthetic namespace entries from descriptors, + // collect descriptor IDs to re-read. for _, iterator := range []func(func(desc catalog.Descriptor) error) error{ tc.uncommitted.iterateUncommittedByID, tc.synthetic.iterateSyntheticByID, - // TODO(postamar): include temporary descriptors? } { _ = iterator(func(desc catalog.Descriptor) error { - ids.Add(desc.GetID()) + descIDs.Add(desc.GetID()) + if sc, ok := desc.(catalog.SchemaDescriptor); ok { + _ = sc.ForEachFunctionOverload(func(overload descpb.SchemaDescriptor_FunctionOverload) error { + descIDs.Add(overload.ID) + return nil + }) + } + if !desc.Dropped() && !desc.SkipNamespace() { + ret.UpsertNamespaceEntry(desc, desc.GetID(), desc.GetModificationTime()) + } return nil }) } + // Add in-memory temporary schema IDs. + if tc.temporarySchemaProvider.HasTemporarySchema() { + tempSchemaName := tc.temporarySchemaProvider.GetTemporarySchemaName() + descIDs.ForEach(func(maybeDatabaseID descpb.ID) { + schemaID := tc.temporarySchemaProvider.GetTemporarySchemaIDForDB(maybeDatabaseID) + if schemaID == descpb.InvalidID { + return + } + ret.UpsertDescriptor(schemadesc.NewTemporarySchema(tempSchemaName, schemaID, maybeDatabaseID)) + }) + } + // Add uncommitted comments and zone configs. + tc.uncommittedComments.addAllToCatalog(ret) + tc.uncommittedZoneConfigs.addAllToCatalog(ret) + // Remove deleted descriptors from consideration, re-read and add the rest. + tc.deletedDescs.ForEach(descIDs.Remove) flags := tree.CommonLookupFlags{ AvoidLeased: true, IncludeOffline: true, IncludeDropped: true, } - // getDescriptorsByID must be used to ensure proper validation hydration etc. - descs, err := tc.getDescriptorsByID(ctx, txn, flags, ids.Ordered()...) + allDescs, err := tc.getDescriptorsByID(ctx, txn, flags, descIDs.Ordered()...) if err != nil { - return nstree.Catalog{}, err + return nstree.MutableCatalog{}, err } - var ret nstree.MutableCatalog - for _, desc := range descs { + for _, desc := range allDescs { ret.UpsertDescriptor(desc) } - return ret.Catalog, nil + // Add the virtual catalog. + tc.virtual.addAllToCatalog(ret) + return ret, nil } -// GetAllDatabaseDescriptors returns all database descriptors visible by the -// transaction, ordered by name. -func (tc *Collection) GetAllDatabaseDescriptors( - ctx context.Context, txn *kv.Txn, -) ([]catalog.DatabaseDescriptor, error) { - - read, err := tc.cr.ScanNamespaceForDatabases(ctx, txn) +// GetAllDescriptorsForDatabase retrieves the complete set of descriptors +// in the requested database. +// Deprecated: prefer GetAllInDatabase. +func (tc *Collection) GetAllDescriptorsForDatabase( + ctx context.Context, txn *kv.Txn, db catalog.DatabaseDescriptor, +) (nstree.Catalog, error) { + // Re-read database descriptor to have the freshest version. + flags := tree.CommonLookupFlags{ + AvoidLeased: true, + IncludeDropped: true, + IncludeOffline: true, + } + var err error + _, db, err = tc.GetImmutableDatabaseByID(ctx, txn, db.GetID(), flags) if err != nil { - return nil, err + return nstree.Catalog{}, err } - var readIDs catalog.DescriptorIDSet - _ = read.ForEachDatabaseNamespaceEntry(func(e nstree.NamespaceEntry) error { - if !tc.isShadowedName(e) { - readIDs.Add(e.GetID()) - } - return nil - }) - const isDescriptorRequired = true - read, err = tc.cr.GetByIDs(ctx, txn, readIDs.Ordered(), isDescriptorRequired, catalog.Database) + c, err := tc.GetAllInDatabase(ctx, txn, db) if err != nil { - return nil, err + return nstree.Catalog{}, err } - var m nstree.NameMap - _ = read.ForEachDescriptor(func(desc catalog.Descriptor) error { - m.Upsert(desc, desc.SkipNamespace()) - return nil - }) - _ = tc.uncommitted.iterateUncommittedByID(func(desc catalog.Descriptor) error { - if desc.DescriptorType() == catalog.Database { - m.Upsert(desc, desc.SkipNamespace()) - } + var ret nstree.MutableCatalog + ret.UpsertDescriptor(db) + _ = c.ForEachDescriptor(func(desc catalog.Descriptor) error { + ret.UpsertDescriptor(desc) return nil }) - _ = tc.synthetic.iterateSyntheticByID(func(desc catalog.Descriptor) error { - if desc.DescriptorType() == catalog.Database { - m.Upsert(desc, desc.SkipNamespace()) + return ret.Catalog, nil +} + +// GetAllDescriptors returns all physical descriptors visible by the +// transaction. +// Deprecated: prefer GetAll. +func (tc *Collection) GetAllDescriptors(ctx context.Context, txn *kv.Txn) (nstree.Catalog, error) { + all, err := tc.GetAll(ctx, txn) + if err != nil { + return nstree.Catalog{}, err + } + var ret nstree.MutableCatalog + _ = all.ForEachDescriptor(func(desc catalog.Descriptor) error { + switch d := desc.(type) { + case catalog.SchemaDescriptor: + switch d.SchemaKind() { + case catalog.SchemaPublic, catalog.SchemaVirtual, catalog.SchemaTemporary: + return nil + } + case catalog.TableDescriptor: + if d.IsVirtualTable() { + return nil + } } + ret.UpsertDescriptor(desc) return nil }) - var ids catalog.DescriptorIDSet - _ = m.IterateDatabasesByName(func(entry catalog.NameEntry) error { - ids.Add(entry.GetID()) - return nil - }) - flags := tree.CommonLookupFlags{ - AvoidLeased: true, - IncludeOffline: true, - IncludeDropped: true, - } - // getDescriptorsByID must be used to ensure proper validation hydration etc. - descs, err := tc.getDescriptorsByID(ctx, txn, flags, ids.Ordered()...) + return ret.Catalog, nil +} + +// GetAllDatabaseDescriptors returns all database descriptors visible by the +// transaction, ordered by name. +// Deprecated: prefer GetAllDatabases. +func (tc *Collection) GetAllDatabaseDescriptors( + ctx context.Context, txn *kv.Txn, +) (ret []catalog.DatabaseDescriptor, _ error) { + c, err := tc.GetAllDatabases(ctx, txn) if err != nil { return nil, err } // Returned slice must be ordered by name. - m.Clear() - dbDescs := make([]catalog.DatabaseDescriptor, 0, len(descs)) - for _, desc := range descs { - m.Upsert(desc, desc.SkipNamespace()) + if err := c.ForEachDatabaseNamespaceEntry(func(e nstree.NamespaceEntry) error { + desc := c.LookupDescriptor(e.GetID()) + db, err := catalog.AsDatabaseDescriptor(desc) + ret = append(ret, db) + return err + }); err != nil { + return nil, err } - _ = m.IterateDatabasesByName(func(entry catalog.NameEntry) error { - dbDescs = append(dbDescs, entry.(catalog.DatabaseDescriptor)) - return nil - }) - return dbDescs, nil + return ret, nil } // GetAllTableDescriptorsInDatabase returns all the table descriptors visible to // the transaction under the database with the given ID. +// Deprecated: prefer GetAllTablesInDatabase. func (tc *Collection) GetAllTableDescriptorsInDatabase( ctx context.Context, txn *kv.Txn, db catalog.DatabaseDescriptor, -) ([]catalog.TableDescriptor, error) { - all, err := tc.GetAllDescriptors(ctx, txn) +) (ret []catalog.TableDescriptor, _ error) { + c, err := tc.GetAllTablesInDatabase(ctx, txn, db) if err != nil { return nil, err } - // Ensure the given ID does indeed belong to a database. - if desc := all.LookupDescriptor(db.GetID()); desc == nil || desc.DescriptorType() != catalog.Database { - return nil, sqlerrors.NewUndefinedDatabaseError(db.GetName()) - } - dbID := db.GetID() - var ret []catalog.TableDescriptor - _ = all.ForEachDescriptor(func(desc catalog.Descriptor) error { - if desc.GetParentID() != dbID { + if err := c.ForEachDescriptor(func(desc catalog.Descriptor) error { + if desc.GetParentID() != db.GetID() { return nil } - if table, ok := desc.(catalog.TableDescriptor); ok { - ret = append(ret, table) - } - return nil - }) + tbl, err := catalog.AsTableDescriptor(desc) + ret = append(ret, tbl) + return err + }); err != nil { + return nil, err + } return ret, nil } // GetSchemasForDatabase returns the schemas for a given database // visible by the transaction. +// Deprecated: prefer GetAllSchemasInDatabase. func (tc *Collection) GetSchemasForDatabase( ctx context.Context, txn *kv.Txn, db catalog.DatabaseDescriptor, ) (map[descpb.ID]string, error) { - read, err := tc.cr.ScanNamespaceForDatabaseSchemas(ctx, txn, db) + c, err := tc.GetAllSchemasInDatabase(ctx, txn, db) if err != nil { return nil, err } ret := make(map[descpb.ID]string) - // This is needed at least for the temp system db during restores. - if !db.HasPublicSchemaWithDescriptor() { - ret[keys.PublicSchemaIDForBackup] = catconstants.PublicSchemaName - } - _ = read.ForEachSchemaNamespaceEntryInDatabase(db.GetID(), func(e nstree.NamespaceEntry) error { - if !tc.isShadowedName(e) { - ret[e.GetID()] = e.GetName() + if err := c.ForEachDescriptor(func(desc catalog.Descriptor) error { + sc, err := catalog.AsSchemaDescriptor(desc) + if err != nil { + return err } - return nil - }) - _ = tc.uncommitted.iterateUncommittedByID(func(desc catalog.Descriptor) error { - if desc.DescriptorType() == catalog.Schema && desc.GetParentID() == db.GetID() { + if sc.SchemaKind() != catalog.SchemaVirtual { ret[desc.GetID()] = desc.GetName() } return nil - }) - _ = tc.synthetic.iterateSyntheticByID(func(desc catalog.Descriptor) error { - if desc.DescriptorType() == catalog.Schema && desc.GetParentID() == db.GetID() { - ret[desc.GetID()] = desc.GetName() - } - return nil - }) + }); err != nil { + return nil, err + } return ret, nil } // GetObjectNamesAndIDs returns the names and IDs of all objects in a schema. +// Deprecated: prefer GetAllObjectsInSchema. func (tc *Collection) GetObjectNamesAndIDs( ctx context.Context, txn *kv.Txn, db catalog.DatabaseDescriptor, sc catalog.SchemaDescriptor, ) (nstree.Catalog, error) { - var mc nstree.MutableCatalog - if sc.SchemaKind() == catalog.SchemaVirtual { - tc.virtual.forEachVirtualObject(sc, func(obj catalog.Descriptor) { - mc.UpsertNamespaceEntry(obj, obj.GetID(), hlc.Timestamp{}) - }) - return mc.Catalog, nil - } - read, err := tc.cr.ScanNamespaceForSchemaObjects(ctx, txn, db, sc) + c, err := tc.GetAllObjectsInSchema(ctx, txn, db, sc) if err != nil { return nstree.Catalog{}, err } - _ = read.ForEachNamespaceEntry(func(e nstree.NamespaceEntry) error { - if !tc.isShadowedName(e) { - mc.UpsertNamespaceEntry(e, e.GetID(), e.GetMVCCTimestamp()) + var ret nstree.MutableCatalog + _ = c.ForEachDescriptor(func(desc catalog.Descriptor) error { + if !desc.Dropped() { + ret.UpsertNamespaceEntry(desc, desc.GetID(), desc.GetModificationTime()) } return nil }) - for _, iterator := range []func(func(catalog.Descriptor) error) error{ - tc.uncommitted.iterateUncommittedByID, - tc.synthetic.iterateSyntheticByID, - } { - _ = iterator(func(desc catalog.Descriptor) error { - if desc.GetParentID() != db.GetID() || desc.GetParentSchemaID() != sc.GetID() { - return nil - } - if desc.Dropped() || desc.SkipNamespace() { - return nil - } - mc.UpsertNamespaceEntry(desc, desc.GetID(), desc.GetModificationTime()) - return nil - }) - } - return mc.Catalog, nil + return ret.Catalog, nil } // SetSyntheticDescriptors sets the provided descriptors as the synthetic diff --git a/pkg/sql/catalog/descs/collection_test.go b/pkg/sql/catalog/descs/collection_test.go index 21cdaf1b3b58..58b8022b486a 100644 --- a/pkg/sql/catalog/descs/collection_test.go +++ b/pkg/sql/catalog/descs/collection_test.go @@ -898,6 +898,10 @@ func formatCatalog(descs []catalog.Descriptor) string { "parent\tschema\tname\tid\tkind\tversion\tdropped\tpublic\n", ) for _, d := range descs { + if d.DescriptorType() != catalog.Database && d.GetParentID() == descpb.InvalidID { + // Skip virtual schemas and virtual tables. + continue + } _, _ = fmt.Fprintf(tr, "%d\t%d\t%s\t%d\t%s\t%d\t%v\t%v\n", d.GetParentID(), d.GetParentSchemaID(), d.GetName(), diff --git a/pkg/sql/catalog/descs/uncommitted_metadata.go b/pkg/sql/catalog/descs/uncommitted_metadata.go index 4d9285aca915..24c7914372ac 100644 --- a/pkg/sql/catalog/descs/uncommitted_metadata.go +++ b/pkg/sql/catalog/descs/uncommitted_metadata.go @@ -16,6 +16,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree" "github.com/cockroachdb/cockroach/pkg/sql/catalog/zone" ) @@ -70,6 +71,12 @@ func (uc *uncommittedComments) upsert(key catalogkeys.CommentKey, cmt string) { uc.uncommitted[key] = cmt } +func (uc *uncommittedComments) addAllToCatalog(mc nstree.MutableCatalog) { + for ck, cmt := range uc.uncommitted { + mc.UpsertComment(ck, cmt) + } +} + type uncommittedZoneConfigs struct { uncommitted map[descpb.ID]catalog.ZoneConfig cachedDescs map[descpb.ID]struct{} @@ -114,3 +121,9 @@ func (uc *uncommittedZoneConfigs) upsert(id descpb.ID, zc *zonepb.ZoneConfig) er uc.uncommitted[id] = zone.NewZoneConfigWithRawBytes(zc, rawBytes) return nil } + +func (uc *uncommittedZoneConfigs) addAllToCatalog(mc nstree.MutableCatalog) { + for id, zc := range uc.uncommitted { + mc.UpsertZoneConfig(id, zc.ZoneConfigProto(), zc.GetRawBytesInStorage()) + } +} diff --git a/pkg/sql/catalog/descs/virtual_descriptors.go b/pkg/sql/catalog/descs/virtual_descriptors.go index a5a216dfccc9..baabee69fc95 100644 --- a/pkg/sql/catalog/descs/virtual_descriptors.go +++ b/pkg/sql/catalog/descs/virtual_descriptors.go @@ -13,9 +13,13 @@ package descs import ( "context" + "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/sql/catalog" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/errors" ) @@ -27,7 +31,7 @@ func makeVirtualDescriptors(schemas catalog.VirtualSchemas) virtualDescriptors { return virtualDescriptors{vs: schemas} } -func (tc *virtualDescriptors) getSchemaByName(schemaName string) catalog.SchemaDescriptor { +func (tc virtualDescriptors) getSchemaByName(schemaName string) catalog.SchemaDescriptor { if tc.vs == nil { return nil } @@ -37,7 +41,7 @@ func (tc *virtualDescriptors) getSchemaByName(schemaName string) catalog.SchemaD return nil } -func (tc *virtualDescriptors) getObjectByName( +func (tc virtualDescriptors) getObjectByName( schema string, object string, flags tree.ObjectLookupFlags, ) (isVirtual bool, _ catalog.Descriptor, _ error) { if tc.vs == nil { @@ -99,14 +103,27 @@ func (tc virtualDescriptors) getSchemaByID( } } -func (tc virtualDescriptors) forEachVirtualObject( - sc catalog.SchemaDescriptor, fn func(obj catalog.Descriptor), -) { - scEntry, ok := tc.vs.GetVirtualSchemaByID(sc.GetID()) - if !ok { - return - } - scEntry.VisitTables(func(object catalog.VirtualObject) { - fn(object.Desc()) +func (tc virtualDescriptors) addAllToCatalog(mc nstree.MutableCatalog) { + _ = tc.vs.Visit(func(vd catalog.Descriptor, comment string) error { + mc.UpsertDescriptor(vd) + if vd.GetID() != keys.PublicSchemaID && !vd.Dropped() && !vd.SkipNamespace() { + mc.UpsertNamespaceEntry(vd, vd.GetID(), hlc.Timestamp{}) + } + if comment == "" { + return nil + } + ck := catalogkeys.CommentKey{ObjectID: uint32(vd.GetID())} + switch vd.DescriptorType() { + case catalog.Database: + ck.CommentType = catalogkeys.DatabaseCommentType + case catalog.Schema: + ck.CommentType = catalogkeys.SchemaCommentType + case catalog.Table: + ck.CommentType = catalogkeys.TableCommentType + default: + return nil + } + mc.UpsertComment(ck, comment) + return nil }) } diff --git a/pkg/sql/catalog/internal/catkv/system_database_cache.go b/pkg/sql/catalog/internal/catkv/system_database_cache.go index ea146488c9df..18ff4c4c2a23 100644 --- a/pkg/sql/catalog/internal/catkv/system_database_cache.go +++ b/pkg/sql/catalog/internal/catkv/system_database_cache.go @@ -58,7 +58,7 @@ func NewSystemDatabaseCache(codec keys.SQLCodec, settings *cluster.Settings) *Sy ) var warm nstree.MutableCatalog _ = ms.ForEachCatalogDescriptor(func(desc catalog.Descriptor) error { - if desc.GetID() < keys.MaxReservedDescID { + if desc.GetID() < keys.MaxReservedDescID && !desc.SkipNamespace() { key := descpb.NameInfo{ ParentID: desc.GetParentID(), ParentSchemaID: desc.GetParentSchemaID(), diff --git a/pkg/sql/catalog/tabledesc/table_desc.go b/pkg/sql/catalog/tabledesc/table_desc.go index 698bd9b70576..2d6841ac0fe2 100644 --- a/pkg/sql/catalog/tabledesc/table_desc.go +++ b/pkg/sql/catalog/tabledesc/table_desc.go @@ -75,7 +75,9 @@ func (desc *wrapper) HasConcurrentSchemaChanges() bool { // SkipNamespace implements the descriptor interface. func (desc *wrapper) SkipNamespace() bool { - return false + // Virtual tables are hard-coded and don't have entries in the + // system.namespace table. + return desc.IsVirtualTable() } // immutable is a custom type for TableDescriptors