From 99824b1dc8ef20fcbbe08212ca5b6db88666f5a8 Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Mon, 12 Dec 2022 11:58:08 -0500 Subject: [PATCH 1/2] sql,resolver: remove Accessor interface This commit refactors the SchemaResolver interface and implementations to get rid of the Accessor. Release note: None. --- pkg/sql/alter_schema.go | 2 +- pkg/sql/catalog/BUILD.bazel | 1 - pkg/sql/catalog/accessor.go | 55 ------------------- pkg/sql/catalog/descs/collection.go | 55 ++++--------------- pkg/sql/catalog/descs/virtual_descriptors.go | 26 +++------ pkg/sql/catalog/resolver/resolver.go | 45 +++++---------- pkg/sql/catalog/resolver/resolver_test.go | 8 +-- pkg/sql/discard.go | 14 ++++- pkg/sql/drop_cascade.go | 5 +- pkg/sql/importer/import_planning.go | 6 +- pkg/sql/importer/import_table_creation.go | 20 ++++--- pkg/sql/opt_catalog.go | 12 +--- pkg/sql/rename_database.go | 45 ++++++--------- pkg/sql/resolver.go | 28 +++++----- pkg/sql/schema_resolver.go | 33 +++++++++-- .../schemachanger/scbuild/builder_state.go | 5 +- pkg/sql/schemachanger/scbuild/dependencies.go | 4 +- pkg/sql/schemachanger/scdeps/BUILD.bazel | 1 + pkg/sql/schemachanger/scdeps/build_deps.go | 26 ++++----- .../scdeps/sctestdeps/test_deps.go | 53 ++++++++---------- pkg/sql/scrub.go | 13 +++-- pkg/sql/sem/tree/name_resolution.go | 7 +-- pkg/sql/temporary_schema.go | 44 ++++++++------- pkg/sql/temporary_schema_test.go | 10 +++- 24 files changed, 199 insertions(+), 319 deletions(-) delete mode 100644 pkg/sql/catalog/accessor.go diff --git a/pkg/sql/alter_schema.go b/pkg/sql/alter_schema.go index 4614c8007870..8b593ea5d82f 100644 --- a/pkg/sql/alter_schema.go +++ b/pkg/sql/alter_schema.go @@ -125,7 +125,7 @@ func (n *alterSchemaNode) startExec(params runParams) error { lookupFlags := tree.CommonLookupFlags{Required: true, AvoidLeased: true} if err := maybeFailOnDependentDescInRename( - params.ctx, params.p, n.db, n.desc.GetName(), lookupFlags, catalog.Schema, + params.ctx, params.p, n.db, n.desc, lookupFlags, catalog.Schema, ); err != nil { return err } diff --git a/pkg/sql/catalog/BUILD.bazel b/pkg/sql/catalog/BUILD.bazel index 572f09773ef8..5881e4da112b 100644 --- a/pkg/sql/catalog/BUILD.bazel +++ b/pkg/sql/catalog/BUILD.bazel @@ -5,7 +5,6 @@ load("//pkg/testutils/buildutil:buildutil.bzl", "disallowed_imports_test") go_library( name = "catalog", srcs = [ - "accessor.go", "catalog.go", "descriptor.go", "descriptor_id_set.go", diff --git a/pkg/sql/catalog/accessor.go b/pkg/sql/catalog/accessor.go deleted file mode 100644 index 2289495e0488..000000000000 --- a/pkg/sql/catalog/accessor.go +++ /dev/null @@ -1,55 +0,0 @@ -// Copyright 2020 The Cockroach Authors. -// -// Use of this software is governed by the Business Source License -// included in the file licenses/BSL.txt. -// -// As of the Change Date specified in that file, in accordance with -// the Business Source License, use of this software will be governed -// by the Apache License, Version 2.0, included in the file -// licenses/APL.txt. - -package catalog - -import ( - "context" - - "github.com/cockroachdb/cockroach/pkg/kv" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" -) - -// Accessor is an implementation used by the SQL layer in order to implement the -// resolver.SchemaResolver interface on top of the planner. Its abstract nature -// is due to its legacy rather than for proper dependency injection. It is only -// implemented by descs.Collection. This status quo is not intended to persist -// throughout the entire 21.2 release. -// -// TODO(ajwerner): Build out a proper layering of interfaces to enable -// dependency injection for descriptor retrieval. -type Accessor interface { - - // GetImmutableDatabaseByName looks up a database by name and returns its - // descriptor. If the database is not found and required is true, - // an error is returned; otherwise a nil reference is returned. - // - // Warning: This method uses no virtual schema information and only exists to - // accommodate the existing resolver.SchemaResolver interface (see #58228). - // Use GetMutableDatabaseByName() and GetImmutableDatabaseByName() on - // descs.Collection instead when possible. - GetImmutableDatabaseByName( - ctx context.Context, txn *kv.Txn, dbName string, flags tree.DatabaseLookupFlags, - ) (DatabaseDescriptor, error) - - // GetObjectNamesAndIDs returns the list of all objects in the given - // database and schema. - // TODO(solon): when separate schemas are supported, this - // API should be extended to use schema descriptors. - // - // TODO(ajwerner,rohany): This API is utilized to support glob patterns that - // are fundamentally sometimes ambiguous (see GRANT and the ambiguity between - // tables and types). Furthermore, the fact that this buffers everything - // in ram in unfortunate. - GetObjectNamesAndIDs( - ctx context.Context, txn *kv.Txn, db DatabaseDescriptor, scName string, flags tree.DatabaseListFlags, - ) (tree.TableNames, descpb.IDs, error) -} diff --git a/pkg/sql/catalog/descs/collection.go b/pkg/sql/catalog/descs/collection.go index a20e25b0bbb0..0f71f6736c7b 100644 --- a/pkg/sql/catalog/descs/collection.go +++ b/pkg/sql/catalog/descs/collection.go @@ -136,8 +136,6 @@ type Collection struct { sqlLivenessSession sqlliveness.Session } -var _ catalog.Accessor = (*Collection)(nil) - // GetDeletedDescs returns the deleted descriptors of the collection. func (tc *Collection) GetDeletedDescs() catalog.DescriptorIDSet { return tc.deletedDescs @@ -907,51 +905,18 @@ func (tc *Collection) GetSchemasForDatabase( return ret, nil } -// GetObjectNamesAndIDs returns the names and IDs of all objects in a database and schema. +// GetObjectNamesAndIDs returns the names and IDs of all objects in a schema. func (tc *Collection) GetObjectNamesAndIDs( - ctx context.Context, - txn *kv.Txn, - db catalog.DatabaseDescriptor, - scName string, - flags tree.DatabaseListFlags, -) (tree.TableNames, descpb.IDs, error) { - if ok, names, ds := tc.virtual.maybeGetObjectNamesAndIDs( - scName, db, flags, - ); ok { - return names, ds, nil - } - - schemaFlags := tree.SchemaLookupFlags{ - Required: flags.Required, - AvoidLeased: flags.RequireMutable || flags.AvoidLeased, - IncludeDropped: flags.IncludeDropped, - IncludeOffline: flags.IncludeOffline, - } - schema, err := tc.getSchemaByName(ctx, txn, db, scName, schemaFlags) - if err != nil { - return nil, nil, err - } - if schema == nil { // required must have been false - return nil, nil, nil - } - read, err := tc.cr.ScanNamespaceForSchemaObjects(ctx, txn, db, schema) - if err != nil { - return nil, nil, err + 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 } - var tableNames tree.TableNames - var tableIDs descpb.IDs - _ = read.ForEachNamespaceEntry(func(e nstree.NamespaceEntry) error { - if e.GetParentID() != db.GetID() || e.GetParentSchemaID() != schema.GetID() { - return nil - } - tn := tree.MakeTableNameWithSchema(tree.Name(db.GetName()), tree.Name(scName), tree.Name(e.GetName())) - tn.ExplicitCatalog = flags.ExplicitPrefix - tn.ExplicitSchema = flags.ExplicitPrefix - tableNames = append(tableNames, tn) - tableIDs = append(tableIDs, e.GetID()) - return nil - }) - return tableNames, tableIDs, nil + return tc.cr.ScanNamespaceForSchemaObjects(ctx, txn, db, sc) } // SetSyntheticDescriptors sets the provided descriptors as the synthetic diff --git a/pkg/sql/catalog/descs/virtual_descriptors.go b/pkg/sql/catalog/descs/virtual_descriptors.go index d344209ae2fb..a5a216dfccc9 100644 --- a/pkg/sql/catalog/descs/virtual_descriptors.go +++ b/pkg/sql/catalog/descs/virtual_descriptors.go @@ -99,26 +99,14 @@ func (tc virtualDescriptors) getSchemaByID( } } -func (tc virtualDescriptors) maybeGetObjectNamesAndIDs( - scName string, dbDesc catalog.DatabaseDescriptor, flags tree.DatabaseListFlags, -) (isVirtual bool, _ tree.TableNames, _ descpb.IDs) { - if tc.vs == nil { - return false, nil, nil - } - entry, ok := tc.vs.GetVirtualSchema(scName) +func (tc virtualDescriptors) forEachVirtualObject( + sc catalog.SchemaDescriptor, fn func(obj catalog.Descriptor), +) { + scEntry, ok := tc.vs.GetVirtualSchemaByID(sc.GetID()) if !ok { - return false, nil, nil + return } - names := make(tree.TableNames, 0, entry.NumTables()) - IDs := make(descpb.IDs, 0, entry.NumTables()) - schemaDesc := entry.Desc() - entry.VisitTables(func(table catalog.VirtualObject) { - name := tree.MakeTableNameWithSchema( - tree.Name(dbDesc.GetName()), tree.Name(schemaDesc.GetName()), tree.Name(table.Desc().GetName())) - name.ExplicitCatalog = flags.ExplicitPrefix - name.ExplicitSchema = flags.ExplicitPrefix - names = append(names, name) - IDs = append(IDs, table.Desc().GetID()) + scEntry.VisitTables(func(object catalog.VirtualObject) { + fn(object.Desc()) }) - return true, names, IDs } diff --git a/pkg/sql/catalog/resolver/resolver.go b/pkg/sql/catalog/resolver/resolver.go index ed98fc065cd7..d075977a07df 100644 --- a/pkg/sql/catalog/resolver/resolver.go +++ b/pkg/sql/catalog/resolver/resolver.go @@ -46,10 +46,18 @@ type SchemaResolver interface { tree.TypeReferenceResolver tree.FunctionReferenceResolver - // Accessor is a crufty name and interface that wraps the *descs.Collection. - Accessor() catalog.Accessor + // GetObjectNamesAndIDs returns the names and IDs of the objects in the + // schema. + GetObjectNamesAndIDs( + ctx context.Context, db catalog.DatabaseDescriptor, sc catalog.SchemaDescriptor, + ) (tree.TableNames, descpb.IDs, error) + + // MustGetCurrentSessionDatabase returns the database descriptor for the + // current session database. + MustGetCurrentSessionDatabase(ctx context.Context) (catalog.DatabaseDescriptor, error) + + // CurrentSearchPath returns the current search path. CurrentSearchPath() sessiondata.SearchPath - CommonLookupFlagsRequired() tree.CommonLookupFlags } // ObjectNameExistingResolver is the helper interface to resolve table @@ -85,25 +93,6 @@ type ObjectNameTargetResolver interface { var ErrNoPrimaryKey = pgerror.Newf(pgcode.NoPrimaryKey, "requested table does not have a primary key") -// GetObjectNamesAndIDs retrieves the names and IDs of all objects in the -// target database/schema. If explicitPrefix is set, the returned -// table names will have an explicit schema and catalog name. -func GetObjectNamesAndIDs( - ctx context.Context, - txn *kv.Txn, - sr SchemaResolver, - codec keys.SQLCodec, - dbDesc catalog.DatabaseDescriptor, - scName string, - explicitPrefix bool, -) (tree.TableNames, descpb.IDs, error) { - flags := tree.DatabaseListFlags{ - CommonLookupFlags: sr.CommonLookupFlagsRequired(), - ExplicitPrefix: explicitPrefix, - } - return sr.Accessor().GetObjectNamesAndIDs(ctx, txn, dbDesc, scName, flags) -} - // ResolveExistingTableObject looks up an existing object. // If required is true, an error is returned if the object does not exist. // Optionally, if a desired descriptor type is specified, that type is checked. @@ -556,8 +545,6 @@ func ResolveIndex( ctx context.Context, schemaResolver SchemaResolver, tableIndexName *tree.TableIndexName, - txn *kv.Txn, - codec keys.SQLCodec, required bool, requireActiveIndex bool, ) ( @@ -624,7 +611,7 @@ func ResolveIndex( } tblFound, tbl, idx, err := findTableContainingIndex( - ctx, tree.Name(tableIndexName.Index), resolvedPrefix, txn, codec, schemaResolver, requireActiveIndex, + ctx, tree.Name(tableIndexName.Index), resolvedPrefix, schemaResolver, requireActiveIndex, ) if err != nil { return false, catalog.ResolvedObjectPrefix{}, nil, nil, err @@ -667,7 +654,7 @@ func ResolveIndex( schemaFound = true candidateFound, tbl, idx, curErr := findTableContainingIndex( - ctx, tree.Name(tableIndexName.Index), candidateResolvedPrefix, txn, codec, schemaResolver, requireActiveIndex, + ctx, tree.Name(tableIndexName.Index), candidateResolvedPrefix, schemaResolver, requireActiveIndex, ) if curErr != nil { return false, catalog.ResolvedObjectPrefix{}, nil, nil, curErr @@ -777,14 +764,10 @@ func findTableContainingIndex( ctx context.Context, indexName tree.Name, resolvedPrefix catalog.ResolvedObjectPrefix, - txn *kv.Txn, - codec keys.SQLCodec, schemaResolver SchemaResolver, requireActiveIndex bool, ) (found bool, tblDesc catalog.TableDescriptor, idxDesc catalog.Index, err error) { - dsNames, _, err := GetObjectNamesAndIDs( - ctx, txn, schemaResolver, codec, resolvedPrefix.Database, resolvedPrefix.Schema.GetName(), true, - ) + dsNames, _, err := schemaResolver.GetObjectNamesAndIDs(ctx, resolvedPrefix.Database, resolvedPrefix.Schema) if err != nil { return false, nil, nil, err diff --git a/pkg/sql/catalog/resolver/resolver_test.go b/pkg/sql/catalog/resolver/resolver_test.go index 31176afb9275..6938e488b542 100644 --- a/pkg/sql/catalog/resolver/resolver_test.go +++ b/pkg/sql/catalog/resolver/resolver_test.go @@ -637,7 +637,7 @@ CREATE TABLE c (a INT, INDEX idx2(a));`, for _, tc := range testCases { t.Run(tc.testName, func(t *testing.T) { _, prefix, tblDesc, idxDesc, err := resolver.ResolveIndex( - ctx, schemaResolver, tc.name, txn, execCfg.Codec, true, false) + ctx, schemaResolver, tc.name, true, false) var res string if err != nil { res = fmt.Sprintf("error: %s", err.Error()) @@ -647,7 +647,7 @@ CREATE TABLE c (a INT, INDEX idx2(a));`, require.Equal(t, tc.expected, res) _, _, _, _, err = resolver.ResolveIndex( - ctx, schemaResolver, tc.name, txn, execCfg.Codec, false, false) + ctx, schemaResolver, tc.name, false, false) if tc.errIfNotRequired { require.Error(t, err) } else { @@ -720,8 +720,6 @@ CREATE INDEX baz_idx ON baz (s); ctx, schemaResolver, newTableIndexName("", "", "", "baz_idx"), - txn, - execCfg.Codec, false, false, ) @@ -735,8 +733,6 @@ CREATE INDEX baz_idx ON baz (s); ctx, schemaResolver, newTableIndexName("", "", "", "foo_idx"), - txn, - execCfg.Codec, false, false, ) diff --git a/pkg/sql/discard.go b/pkg/sql/discard.go index 27085f728735..980dc59ca56d 100644 --- a/pkg/sql/discard.go +++ b/pkg/sql/discard.go @@ -93,9 +93,17 @@ func deleteTempTables(ctx context.Context, p *planner) error { if err != nil { return err } - for _, dbDesc := range allDbDescs { - schemaName := p.TemporarySchemaName() - err = cleanupSchemaObjects(ctx, p.Txn(), descCol, codec, ie, dbDesc, schemaName) + flags := p.CommonLookupFlagsRequired() + flags.Required = false + for _, db := range allDbDescs { + sc, err := descCol.GetImmutableSchemaByName(ctx, p.Txn(), db, p.TemporarySchemaName(), flags) + if err != nil { + return err + } + if sc == nil { + continue + } + err = cleanupTempSchemaObjects(ctx, p.Txn(), descCol, codec, ie, db, sc) if err != nil { return err } diff --git a/pkg/sql/drop_cascade.go b/pkg/sql/drop_cascade.go index 7e65c4d42bdf..929a78a66777 100644 --- a/pkg/sql/drop_cascade.go +++ b/pkg/sql/drop_cascade.go @@ -18,7 +18,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/funcdesc" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" @@ -59,9 +58,7 @@ func newDropCascadeState() *dropCascadeState { func (d *dropCascadeState) collectObjectsInSchema( ctx context.Context, p *planner, db *dbdesc.Mutable, schema catalog.SchemaDescriptor, ) error { - names, _, err := resolver.GetObjectNamesAndIDs( - ctx, p.txn, p, p.ExecCfg().Codec, db, schema.GetName(), true, /* explicitPrefix */ - ) + names, _, err := p.GetObjectNamesAndIDs(ctx, db, schema) if err != nil { return err } diff --git a/pkg/sql/importer/import_planning.go b/pkg/sql/importer/import_planning.go index c52100fc70e9..dada66c8f5b3 100644 --- a/pkg/sql/importer/import_planning.go +++ b/pkg/sql/importer/import_planning.go @@ -520,11 +520,7 @@ func importPlanHook( } else { // No target table means we're importing whatever we find into the session // database, so it must exist. - txn := p.Txn() - db, err = p.Accessor().GetImmutableDatabaseByName(ctx, txn, p.SessionData().Database, tree.DatabaseLookupFlags{ - AvoidLeased: true, - Required: true, - }) + db, err = p.MustGetCurrentSessionDatabase(ctx) if err != nil { return pgerror.Wrap(err, pgcode.UndefinedObject, "could not resolve current database") diff --git a/pkg/sql/importer/import_table_creation.go b/pkg/sql/importer/import_table_creation.go index 54b63352d5c7..31b39b5b5c57 100644 --- a/pkg/sql/importer/import_table_creation.go +++ b/pkg/sql/importer/import_table_creation.go @@ -348,9 +348,18 @@ type fkResolver struct { var _ resolver.SchemaResolver = &fkResolver{} -// Accessor implements the resolver.SchemaResolver interface. -func (r *fkResolver) Accessor() catalog.Accessor { - return nil +// GetObjectNamesAndIDs implements the resolver.SchemaResolver interface. +func (r *fkResolver) GetObjectNamesAndIDs( + ctx context.Context, db catalog.DatabaseDescriptor, sc catalog.SchemaDescriptor, +) (tree.TableNames, descpb.IDs, error) { + return nil, nil, errors.AssertionFailedf("GetObjectNamesAndIDs is not implemented by fkResolver") +} + +// MustGetCurrentSessionDatabase implements the resolver.SchemaResolver interface. +func (r *fkResolver) MustGetCurrentSessionDatabase( + ctx context.Context, +) (catalog.DatabaseDescriptor, error) { + return nil, errors.AssertionFailedf("MustGetCurrentSessionDatabase is not implemented by fkResolver") } // CurrentDatabase implements the resolver.SchemaResolver interface. @@ -363,11 +372,6 @@ func (r *fkResolver) CurrentSearchPath() sessiondata.SearchPath { return sessiondata.SearchPath{} } -// CommonLookupFlagsRequired implements the resolver.SchemaResolver interface. -func (r *fkResolver) CommonLookupFlagsRequired() tree.CommonLookupFlags { - return tree.CommonLookupFlags{Required: true} -} - // LookupObject implements the tree.ObjectNameExistingResolver interface. func (r *fkResolver) LookupObject( ctx context.Context, flags tree.ObjectLookupFlags, dbName, scName, obName string, diff --git a/pkg/sql/opt_catalog.go b/pkg/sql/opt_catalog.go index d2e9531cf14e..54a8c3b1e7e2 100644 --- a/pkg/sql/opt_catalog.go +++ b/pkg/sql/opt_catalog.go @@ -147,15 +147,7 @@ func (os *optSchema) Name() *cat.SchemaName { func (os *optSchema) GetDataSourceNames( ctx context.Context, ) ([]cat.DataSourceName, descpb.IDs, error) { - return resolver.GetObjectNamesAndIDs( - ctx, - os.planner.Txn(), - os.planner, - os.planner.ExecCfg().Codec, - os.database, - os.name.Schema(), - true, /* explicitPrefix */ - ) + return os.planner.GetObjectNamesAndIDs(ctx, os.database, os.schema) } func (os *optSchema) getDescriptorForPermissionsCheck() catalog.Descriptor { @@ -270,8 +262,6 @@ func (oc *optCatalog) ResolveIndex( ctx, oc.planner, name, - oc.planner.Txn(), - oc.planner.EvalContext().Codec, true, /* required */ true, /* requireActiveIndex */ ) diff --git a/pkg/sql/rename_database.go b/pkg/sql/rename_database.go index 8947356fb23d..2cca0d1f98b8 100644 --- a/pkg/sql/rename_database.go +++ b/pkg/sql/rename_database.go @@ -127,7 +127,11 @@ func (n *renameDatabaseNode) startExec(params runParams) error { return err } for _, schema := range schemas { - if err := maybeFailOnDependentDescInRename(ctx, p, dbDesc, schema, lookupFlags, catalog.Database); err != nil { + sc, err := p.Descriptors().GetImmutableSchemaByName(ctx, p.txn, dbDesc, schema, lookupFlags) + if err != nil { + return err + } + if err := maybeFailOnDependentDescInRename(ctx, p, dbDesc, sc, lookupFlags, catalog.Database); err != nil { return err } } @@ -197,35 +201,22 @@ func getQualifiedDependentObjectName( func maybeFailOnDependentDescInRename( ctx context.Context, p *planner, - dbDesc catalog.DatabaseDescriptor, - schema string, + db catalog.DatabaseDescriptor, + sc catalog.SchemaDescriptor, lookupFlags tree.CommonLookupFlags, renameDescType catalog.DescriptorType, ) error { - tbNames, _, err := p.Descriptors().GetObjectNamesAndIDs( - ctx, - p.txn, - dbDesc, - schema, - tree.DatabaseListFlags{ - CommonLookupFlags: lookupFlags, - ExplicitPrefix: true, - }, - ) + _, ids, err := p.GetObjectNamesAndIDs(ctx, db, sc) if err != nil { return err } - lookupFlags.Required = false - // TODO(ajwerner): Make this do something better than one-at-a-time lookups - // followed by catalogkv reads on each dependency. - for i := range tbNames { - found, tbDesc, err := p.Descriptors().GetImmutableTableByName( - ctx, p.txn, &tbNames[i], tree.ObjectLookupFlags{CommonLookupFlags: lookupFlags}, - ) - if err != nil { - return err - } - if !found { + descs, err := p.Descriptors().GetImmutableDescriptorsByID(ctx, p.txn, lookupFlags, ids...) + if err != nil { + return err + } + for _, desc := range descs { + tbDesc, ok := desc.(catalog.TableDescriptor) + if !ok { continue } @@ -243,12 +234,12 @@ func maybeFailOnDependentDescInRename( } tbTableName := tree.MakeTableNameWithSchema( - tree.Name(dbDesc.GetName()), - tree.Name(schema), + tree.Name(db.GetName()), + tree.Name(sc.GetName()), tree.Name(tbDesc.GetName()), ) dependentDescQualifiedString, err := getQualifiedDependentObjectName( - ctx, p, dbDesc.GetName(), schema, tbDesc, dependentDesc, + ctx, p, db.GetName(), sc.GetName(), tbDesc, dependentDesc, ) if err != nil { return err diff --git a/pkg/sql/resolver.go b/pkg/sql/resolver.go index 1608bac5b152..6956aec18d34 100644 --- a/pkg/sql/resolver.go +++ b/pkg/sql/resolver.go @@ -16,7 +16,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/keys" - "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/sql/catalog" @@ -448,21 +447,25 @@ func (p *planner) getDescriptorsFromTargetListForPrivilegeChange( if targets.AllTablesInSchema || targets.AllSequencesInSchema { // Get all the descriptors for the tables in the specified schemas. var descs []DescriptorWithObjectType - for _, sc := range targets.Schemas { + for _, scName := range targets.Schemas { dbName := p.CurrentDatabase() - if sc.ExplicitCatalog { - dbName = sc.Catalog() + if scName.ExplicitCatalog { + dbName = scName.Catalog() } db, err := p.Descriptors().GetMutableDatabaseByName(ctx, p.txn, dbName, flags) if err != nil { return nil, err } - _, objectIDs, err := resolver.GetObjectNamesAndIDs( - ctx, p.txn, p, p.ExecCfg().Codec, db, sc.Schema(), true, /* explicitPrefix */ + sc, err := p.Descriptors().GetImmutableSchemaByName( + ctx, p.txn, db, scName.Schema(), p.CommonLookupFlagsRequired(), ) if err != nil { return nil, err } + _, objectIDs, err := p.GetObjectNamesAndIDs(ctx, db, sc) + if err != nil { + return nil, err + } muts, err := p.Descriptors().GetMutableDescriptorsByID(ctx, p.txn, objectIDs...) if err != nil { return nil, err @@ -735,18 +738,13 @@ func expandMutableIndexName( ctx context.Context, p *planner, index *tree.TableIndexName, requireTable bool, ) (tn *tree.TableName, desc *tabledesc.Mutable, err error) { p.runWithOptions(resolveFlags{skipCache: true}, func() { - tn, desc, err = expandIndexName(ctx, p.txn, p, p.ExecCfg().Codec, index, requireTable) + tn, desc, err = expandIndexName(ctx, p, index, requireTable) }) return tn, desc, err } func expandIndexName( - ctx context.Context, - txn *kv.Txn, - p *planner, - codec keys.SQLCodec, - index *tree.TableIndexName, - requireTable bool, + ctx context.Context, p *planner, index *tree.TableIndexName, requireTable bool, ) (tn *tree.TableName, desc *tabledesc.Mutable, err error) { tn = &index.Table if tn.Object() != "" { @@ -762,7 +760,7 @@ func expandIndexName( return tn, desc, nil } - found, resolvedPrefix, tbl, _, err := resolver.ResolveIndex(ctx, p, index, txn, codec, requireTable, false /*requireActiveIndex*/) + found, resolvedPrefix, tbl, _, err := resolver.ResolveIndex(ctx, p, index, requireTable, false /*requireActiveIndex*/) if err != nil { return nil, nil, err } @@ -806,7 +804,7 @@ func (p *planner) getTableAndIndexImpl( ctx context.Context, tableWithIndex *tree.TableIndexName, privilege privilege.Kind, ) (catalog.ResolvedObjectPrefix, *tabledesc.Mutable, catalog.Index, error) { _, resolvedPrefix, tbl, idx, err := resolver.ResolveIndex( - ctx, p, tableWithIndex, p.Txn(), p.EvalContext().Codec, true /* required */, true, /* requireActiveIndex */ + ctx, p, tableWithIndex, true /* required */, true, /* requireActiveIndex */ ) if err != nil { return catalog.ResolvedObjectPrefix{}, nil, nil, err diff --git a/pkg/sql/schema_resolver.go b/pkg/sql/schema_resolver.go index a1fdcae8e20c..df94d626e2fa 100644 --- a/pkg/sql/schema_resolver.go +++ b/pkg/sql/schema_resolver.go @@ -21,6 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/funcdesc" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree" "github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" @@ -65,9 +66,32 @@ type schemaResolver struct { typeResolutionDbID descpb.ID } -// Accessor implements the resolver.SchemaResolver interface. -func (sr *schemaResolver) Accessor() catalog.Accessor { - return sr.descCollection +// GetObjectNamesAndIDs implements the resolver.SchemaResolver interface. +func (sr *schemaResolver) GetObjectNamesAndIDs( + ctx context.Context, db catalog.DatabaseDescriptor, sc catalog.SchemaDescriptor, +) (tableNames tree.TableNames, tableIDs descpb.IDs, _ error) { + c, err := sr.descCollection.GetObjectNamesAndIDs(ctx, sr.txn, db, sc) + if err != nil { + return nil, nil, err + } + _ = c.ForEachNamespaceEntry(func(e nstree.NamespaceEntry) error { + tn := tree.MakeTableNameWithSchema(tree.Name(db.GetName()), tree.Name(sc.GetName()), tree.Name(e.GetName())) + tableNames = append(tableNames, tn) + tableIDs = append(tableIDs, e.GetID()) + return nil + }) + return tableNames, tableIDs, nil +} + +// MustGetCurrentSessionDatabase implements the resolver.SchemaResolver interface. +func (sr *schemaResolver) MustGetCurrentSessionDatabase( + ctx context.Context, +) (catalog.DatabaseDescriptor, error) { + flags := tree.DatabaseLookupFlags{ + AvoidLeased: true, + Required: true, + } + return sr.descCollection.GetImmutableDatabaseByName(ctx, sr.txn, sr.CurrentDatabase(), flags) } // CurrentSearchPath implements the resolver.SchemaResolver interface. @@ -75,7 +99,8 @@ func (sr *schemaResolver) CurrentSearchPath() sessiondata.SearchPath { return sr.sessionDataStack.Top().SearchPath } -// CommonLookupFlagsRequired implements the resolver.SchemaResolver interface. +// CommonLookupFlagsRequired is a convenience method for returning a +// default set of tree.CommonLookupFlags. func (sr *schemaResolver) CommonLookupFlagsRequired() tree.CommonLookupFlags { return tree.CommonLookupFlags{ Required: true, diff --git a/pkg/sql/schemachanger/scbuild/builder_state.go b/pkg/sql/schemachanger/scbuild/builder_state.go index 8f1ec304da08..5e50ab7fae1b 100644 --- a/pkg/sql/schemachanger/scbuild/builder_state.go +++ b/pkg/sql/schemachanger/scbuild/builder_state.go @@ -1001,10 +1001,7 @@ func (b *builderState) ensureDescriptor(id catid.DescID) { // Handle special case of schema children, which have to be added to // the back-referenced ID set but which aren't explicitly referenced in // the schema descriptor itself. - _, objectIDs := b.cr.ReadObjectNamesAndIDs(b.ctx, db.(catalog.DatabaseDescriptor), d) - for _, objectID := range objectIDs { - c.backrefs.Add(objectID) - } + b.cr.ReadObjectIDs(b.ctx, db.(catalog.DatabaseDescriptor), d).ForEach(c.backrefs.Add) if err := d.ForEachFunctionOverload(func(overload descpb.SchemaDescriptor_FunctionOverload) error { c.backrefs.Add(overload.ID) return nil diff --git a/pkg/sql/schemachanger/scbuild/dependencies.go b/pkg/sql/schemachanger/scbuild/dependencies.go index 65a8bb685f4e..73b421cc58fc 100644 --- a/pkg/sql/schemachanger/scbuild/dependencies.go +++ b/pkg/sql/schemachanger/scbuild/dependencies.go @@ -133,8 +133,8 @@ type CatalogReader interface { found bool, prefix catalog.ResolvedObjectPrefix, tbl catalog.TableDescriptor, idx catalog.Index, ) - // ReadObjectNamesAndIDs looks up the namespace entries for a schema. - ReadObjectNamesAndIDs(ctx context.Context, db catalog.DatabaseDescriptor, schema catalog.SchemaDescriptor) (tree.TableNames, descpb.IDs) + // ReadObjectIDs looks up the IDs of all objects in a schema. + ReadObjectIDs(ctx context.Context, db catalog.DatabaseDescriptor, schema catalog.SchemaDescriptor) catalog.DescriptorIDSet // MustReadDescriptor looks up a descriptor by ID. MustReadDescriptor(ctx context.Context, id descpb.ID) catalog.Descriptor diff --git a/pkg/sql/schemachanger/scdeps/BUILD.bazel b/pkg/sql/schemachanger/scdeps/BUILD.bazel index 1c47e30bc1a7..298b0f5bef15 100644 --- a/pkg/sql/schemachanger/scdeps/BUILD.bazel +++ b/pkg/sql/schemachanger/scdeps/BUILD.bazel @@ -27,6 +27,7 @@ go_library( "//pkg/sql/catalog/catpb", "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/descs", + "//pkg/sql/catalog/nstree", "//pkg/sql/catalog/resolver", "//pkg/sql/oppurpose", "//pkg/sql/rowenc", diff --git a/pkg/sql/schemachanger/scdeps/build_deps.go b/pkg/sql/schemachanger/scdeps/build_deps.go index 9502f9e14d2e..8f5dd7c9a8c4 100644 --- a/pkg/sql/schemachanger/scdeps/build_deps.go +++ b/pkg/sql/schemachanger/scdeps/build_deps.go @@ -22,6 +22,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree" "github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver" "github.com/cockroachdb/cockroach/pkg/sql/rowenc" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scbuild" @@ -169,7 +170,7 @@ func (d *buildDeps) MayResolveIndex( idx catalog.Index, ) { found, prefix, tbl, idx, err := resolver.ResolveIndex( - ctx, d.schemaResolver, &tableIndexName, d.txn, d.codec, false /* required */, false, /* requireActiveIndex */ + ctx, d.schemaResolver, &tableIndexName, false /* required */, false, /* requireActiveIndex */ ) if err != nil { panic(err) @@ -177,24 +178,19 @@ func (d *buildDeps) MayResolveIndex( return found, prefix, tbl, idx } -// ReadObjectNamesAndIDs implements the scbuild.CatalogReader interface. -func (d *buildDeps) ReadObjectNamesAndIDs( +// ReadObjectIDs implements the scbuild.CatalogReader interface. +func (d *buildDeps) ReadObjectIDs( ctx context.Context, db catalog.DatabaseDescriptor, schema catalog.SchemaDescriptor, -) (tree.TableNames, descpb.IDs) { - names, ids, err := d.descsCollection.GetObjectNamesAndIDs(ctx, d.txn, db, schema.GetName(), tree.DatabaseListFlags{ - CommonLookupFlags: tree.CommonLookupFlags{ - Required: true, - RequireMutable: false, - AvoidLeased: true, - IncludeOffline: true, - IncludeDropped: true, - }, - ExplicitPrefix: true, - }) +) (ret catalog.DescriptorIDSet) { + c, err := d.descsCollection.GetObjectNamesAndIDs(ctx, d.txn, db, schema) if err != nil { panic(err) } - return names, ids + _ = c.ForEachNamespaceEntry(func(e nstree.NamespaceEntry) error { + ret.Add(e.GetID()) + return nil + }) + return ret } // ResolveType implements the scbuild.CatalogReader interface. diff --git a/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go b/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go index c9bfdf7ff645..f56c7481e851 100644 --- a/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go +++ b/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go @@ -295,12 +295,7 @@ func (s *TestState) MayResolveType( // MayResolveIndex implements the scbuild.CatalogReader interface. func (s *TestState) MayResolveIndex( ctx context.Context, tableIndexName tree.TableIndexName, -) ( - found bool, - prefix catalog.ResolvedObjectPrefix, - tbl catalog.TableDescriptor, - idx catalog.Index, -) { +) (found bool, _ catalog.ResolvedObjectPrefix, _ catalog.TableDescriptor, _ catalog.Index) { if tableIndexName.Table.Object() != "" { prefix, tbl := s.MayResolveTable(ctx, *tableIndexName.Table.ToUnresolvedObjectName()) if tbl == nil { @@ -314,14 +309,24 @@ func (s *TestState) MayResolveIndex( } db, schema := s.mayResolvePrefix(tableIndexName.Table.ObjectNamePrefix) - dsNames, _ := s.ReadObjectNamesAndIDs(ctx, db.(catalog.DatabaseDescriptor), schema.(catalog.SchemaDescriptor)) - for _, dsName := range dsNames { - prefix, tbl := s.MayResolveTable(ctx, *dsName.ToUnresolvedObjectName()) - if tbl == nil { + prefix := catalog.ResolvedObjectPrefix{ + ExplicitDatabase: true, + ExplicitSchema: true, + Database: db.(catalog.DatabaseDescriptor), + Schema: schema.(catalog.SchemaDescriptor), + } + objectIDs := s.ReadObjectIDs(ctx, prefix.Database, prefix.Schema) + for _, objectID := range objectIDs.Ordered() { + desc, _ := s.mustReadImmutableDescriptor(objectID) + if desc == nil { continue } - idx, err := tbl.FindNonDropIndexWithName(string(tableIndexName.Index)) - if err == nil { + tbl, ok := desc.(catalog.TableDescriptor) + if !ok { + continue + } + idx, _ := tbl.FindNonDropIndexWithName(string(tableIndexName.Index)) + if idx != nil { return true, prefix, tbl, idx } } @@ -431,29 +436,17 @@ func (s *TestState) mayGetByName( return desc } -// ReadObjectNamesAndIDs implements the scbuild.CatalogReader interface. -func (s *TestState) ReadObjectNamesAndIDs( - ctx context.Context, db catalog.DatabaseDescriptor, schema catalog.SchemaDescriptor, -) (names tree.TableNames, ids descpb.IDs) { - m := make(map[string]descpb.ID) +// ReadObjectIDs implements the scbuild.CatalogReader interface. +func (s *TestState) ReadObjectIDs( + _ context.Context, db catalog.DatabaseDescriptor, schema catalog.SchemaDescriptor, +) (ret catalog.DescriptorIDSet) { _ = s.uncommitted.ForEachNamespaceEntry(func(e nstree.NamespaceEntry) error { if e.GetParentID() == db.GetID() && e.GetParentSchemaID() == schema.GetID() { - m[e.GetName()] = e.GetID() - names = append(names, tree.MakeTableNameWithSchema( - tree.Name(db.GetName()), - tree.Name(schema.GetName()), - tree.Name(e.GetName()), - )) + ret.Add(e.GetID()) } return nil }) - sort.Slice(names, func(i, j int) bool { - return names[i].Object() < names[j].Object() - }) - for _, name := range names { - ids = append(ids, m[name.Object()]) - } - return names, ids + return ret } // ResolveType implements the scbuild.CatalogReader interface. diff --git a/pkg/sql/scrub.go b/pkg/sql/scrub.go index 1c8eccff6320..0f9e4fc16b6c 100644 --- a/pkg/sql/scrub.go +++ b/pkg/sql/scrub.go @@ -16,7 +16,6 @@ import ( "strings" "github.com/cockroachdb/cockroach/pkg/sql/catalog" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" @@ -153,25 +152,29 @@ func (n *scrubNode) Close(ctx context.Context) { func (n *scrubNode) startScrubDatabase(ctx context.Context, p *planner, name *tree.Name) error { // Check that the database exists. database := string(*name) - dbDesc, err := p.Descriptors().GetImmutableDatabaseByName(ctx, p.txn, + db, err := p.Descriptors().GetImmutableDatabaseByName(ctx, p.txn, database, tree.DatabaseLookupFlags{Required: true}) if err != nil { return err } - schemas, err := p.Descriptors().GetSchemasForDatabase(ctx, p.txn, dbDesc) + schemas, err := p.Descriptors().GetSchemasForDatabase(ctx, p.txn, db) if err != nil { return err } var tbNames tree.TableNames for _, schema := range schemas { - toAppend, _, err := resolver.GetObjectNamesAndIDs( - ctx, p.txn, p, p.ExecCfg().Codec, dbDesc, schema, true, /*explicitPrefix*/ + sc, err := p.Descriptors().GetImmutableSchemaByName( + ctx, p.txn, db, schema, p.CommonLookupFlagsRequired(), ) if err != nil { return err } + toAppend, _, err := p.GetObjectNamesAndIDs(ctx, db, sc) + if err != nil { + return err + } tbNames = append(tbNames, toAppend...) } diff --git a/pkg/sql/sem/tree/name_resolution.go b/pkg/sql/sem/tree/name_resolution.go index c53a69e6e756..ac8910cbb481 100644 --- a/pkg/sql/sem/tree/name_resolution.go +++ b/pkg/sql/sem/tree/name_resolution.go @@ -188,12 +188,7 @@ type SchemaLookupFlags = CommonLookupFlags type DatabaseLookupFlags = CommonLookupFlags // DatabaseListFlags is the flag struct suitable for GetObjectNamesAndIDs(). -type DatabaseListFlags struct { - CommonLookupFlags - // ExplicitPrefix, when set, will cause the returned table names to - // have an explicit schema and catalog part. - ExplicitPrefix bool -} +type DatabaseListFlags = CommonLookupFlags // DesiredObjectKind represents what kind of object is desired in a name // resolution attempt. diff --git a/pkg/sql/temporary_schema.go b/pkg/sql/temporary_schema.go index c96d55fb5243..fe50a46857b1 100644 --- a/pkg/sql/temporary_schema.go +++ b/pkg/sql/temporary_schema.go @@ -27,6 +27,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree" "github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver" "github.com/cockroachdb/cockroach/pkg/sql/clusterunique" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" @@ -175,14 +176,22 @@ func cleanupSessionTempObjects( return err } for _, dbDesc := range allDbDescs { - if err := cleanupSchemaObjects( + flags := tree.CommonLookupFlags{AvoidLeased: true} + tempSchema, err := descsCol.GetImmutableSchemaByName(ctx, txn, dbDesc, tempSchemaName, flags) + if err != nil { + return err + } + if tempSchema == nil { + continue + } + if err := cleanupTempSchemaObjects( ctx, txn, descsCol, codec, ie, dbDesc, - tempSchemaName, + tempSchema, ); err != nil { return err } @@ -205,29 +214,23 @@ func cleanupSessionTempObjects( }) } -// cleanupSchemaObjects removes all objects that is located within a dbID and schema. +// cleanupTempSchemaObjects removes all objects that is located within a dbID and schema. // // TODO(postamar): properly use descsCol // We're currently unable to leverage descsCol properly because we run DROP // statements in the transaction which cause descsCol's cached state to become // invalid. We should either drop all objects programmatically via descsCol's // API or avoid it entirely. -func cleanupSchemaObjects( +func cleanupTempSchemaObjects( ctx context.Context, txn *kv.Txn, descsCol *descs.Collection, codec keys.SQLCodec, ie sqlutil.InternalExecutor, - dbDesc catalog.DatabaseDescriptor, - schemaName string, + db catalog.DatabaseDescriptor, + sc catalog.SchemaDescriptor, ) error { - tbNames, tbIDs, err := descsCol.GetObjectNamesAndIDs( - ctx, - txn, - dbDesc, - schemaName, - tree.DatabaseListFlags{CommonLookupFlags: tree.CommonLookupFlags{Required: false}}, - ) + c, err := descsCol.GetObjectNamesAndIDs(ctx, txn, db, sc) if err != nil { return err } @@ -242,16 +245,16 @@ func cleanupSchemaObjects( var views descpb.IDs var sequences descpb.IDs - tblDescsByID := make(map[descpb.ID]catalog.TableDescriptor, len(tbNames)) - tblNamesByID := make(map[descpb.ID]tree.TableName, len(tbNames)) - for i, tbName := range tbNames { - desc, err := descsCol.Direct().MustGetTableDescByID(ctx, txn, tbIDs[i]) + tblDescsByID := make(map[descpb.ID]catalog.TableDescriptor) + tblNamesByID := make(map[descpb.ID]tree.TableName) + _ = c.ForEachNamespaceEntry(func(e nstree.NamespaceEntry) error { + desc, err := descsCol.Direct().MustGetTableDescByID(ctx, txn, e.GetID()) if err != nil { return err } tblDescsByID[desc.GetID()] = desc - tblNamesByID[desc.GetID()] = tbName + tblNamesByID[desc.GetID()] = tree.MakeTableNameWithSchema(tree.Name(db.GetName()), tree.Name(sc.GetName()), tree.Name(e.GetName())) databaseIDToTempSchemaID[uint32(desc.GetParentID())] = uint32(desc.GetParentSchemaID()) @@ -267,9 +270,10 @@ func cleanupSchemaObjects( } else if !desc.IsSequence() { tables = append(tables, desc.GetID()) } - } + return nil + }) - searchPath := sessiondata.DefaultSearchPathForUser(username.RootUserName()).WithTemporarySchemaName(schemaName) + searchPath := sessiondata.DefaultSearchPathForUser(username.RootUserName()).WithTemporarySchemaName(sc.GetName()) override := sessiondata.InternalExecutorOverride{ SearchPath: &searchPath, User: username.RootUserName(), diff --git a/pkg/sql/temporary_schema_test.go b/pkg/sql/temporary_schema_test.go index f03c968bcc9d..a01181c58716 100644 --- a/pkg/sql/temporary_schema_test.go +++ b/pkg/sql/temporary_schema_test.go @@ -25,6 +25,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/lease" "github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants" + "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlutil" "github.com/cockroachdb/cockroach/pkg/sql/tests" "github.com/cockroachdb/cockroach/pkg/testutils" @@ -109,14 +110,19 @@ INSERT INTO perm_table VALUES (DEFAULT, 1); if err != nil { return err } - return cleanupSchemaObjects( + flags := tree.CommonLookupFlags{Required: true, AvoidLeased: true} + tempSchema, err := descsCol.GetImmutableSchemaByName(ctx, txn, defaultDB, tempSchemaName, flags) + if err != nil { + return err + } + return cleanupTempSchemaObjects( ctx, txn, descsCol, execCfg.Codec, ie, defaultDB, - tempSchemaName, + tempSchema, ) })) From 5519206f485c87b624ad1967b38d85e30296a316 Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Mon, 12 Dec 2022 14:57:49 -0500 Subject: [PATCH 2/2] descs: fix incorrect namespace shadowing Recent work towards #88571 has made the descriptors collection aware of namespace operations. This awareness erroneously did not extend to operations involving cached scans of the namespace table. Fixes #93002. Release note: None --- pkg/sql/catalog/descs/collection.go | 57 +++++++++++++++++-- pkg/sql/catalog/descs/descriptor.go | 4 +- .../testdata/logic_test/rename_atomic | 14 +++++ 3 files changed, 68 insertions(+), 7 deletions(-) diff --git a/pkg/sql/catalog/descs/collection.go b/pkg/sql/catalog/descs/collection.go index 0f71f6736c7b..37c744b92e15 100644 --- a/pkg/sql/catalog/descs/collection.go +++ b/pkg/sql/catalog/descs/collection.go @@ -389,6 +389,24 @@ func (tc *Collection) markAsShadowedName(id descpb.ID) { }] = struct{}{} } +func (tc *Collection) isShadowedName(nameKey catalog.NameKey) bool { + var k descpb.NameInfo + switch t := nameKey.(type) { + case descpb.NameInfo: + k = t + case *descpb.NameInfo: + k = *t + default: + k = descpb.NameInfo{ + ParentID: nameKey.GetParentID(), + ParentSchemaID: nameKey.GetParentSchemaID(), + Name: nameKey.GetName(), + } + } + _, ok := tc.shadowedNames[k] + return ok +} + // WriteCommentToBatch adds the comment changes to uncommitted layer and writes // to the kv batch. func (tc *Collection) WriteCommentToBatch( @@ -613,7 +631,7 @@ func (tc *Collection) lookupDescriptorID( return objInMemory.GetID(), nil } // Look up ID in storage if nothing was found in memory. - if _, ok := tc.shadowedNames[key]; ok { + if tc.isShadowedName(key) { return descpb.InvalidID, nil } read, err := tc.cr.GetByNames(ctx, txn, []descpb.NameInfo{key}) @@ -690,7 +708,7 @@ func (tc *Collection) GetAllDescriptorsForDatabase( var ids catalog.DescriptorIDSet ids.Add(db.GetID()) _ = read.ForEachNamespaceEntry(func(e nstree.NamespaceEntry) error { - if e.GetParentID() != db.GetID() { + if e.GetParentID() != db.GetID() || tc.isShadowedName(e) { return nil } if !strings.HasPrefix(e.GetName(), catconstants.PgTempSchemaName) && @@ -792,7 +810,9 @@ func (tc *Collection) GetAllDatabaseDescriptors( } var readIDs catalog.DescriptorIDSet _ = read.ForEachDatabaseNamespaceEntry(func(e nstree.NamespaceEntry) error { - readIDs.Add(e.GetID()) + if !tc.isShadowedName(e) { + readIDs.Add(e.GetID()) + } return nil }) const isDescriptorRequired = true @@ -887,7 +907,9 @@ func (tc *Collection) GetSchemasForDatabase( ret[keys.PublicSchemaIDForBackup] = catconstants.PublicSchemaName } _ = read.ForEachSchemaNamespaceEntryInDatabase(db.GetID(), func(e nstree.NamespaceEntry) error { - ret[e.GetID()] = e.GetName() + if !tc.isShadowedName(e) { + ret[e.GetID()] = e.GetName() + } return nil }) _ = tc.uncommitted.iterateUncommittedByID(func(desc catalog.Descriptor) error { @@ -916,7 +938,32 @@ func (tc *Collection) GetObjectNamesAndIDs( }) return mc.Catalog, nil } - return tc.cr.ScanNamespaceForSchemaObjects(ctx, txn, db, sc) + read, err := tc.cr.ScanNamespaceForSchemaObjects(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()) + } + 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 } // SetSyntheticDescriptors sets the provided descriptors as the synthetic diff --git a/pkg/sql/catalog/descs/descriptor.go b/pkg/sql/catalog/descs/descriptor.go index a5174f34aeff..6732a68096c3 100644 --- a/pkg/sql/catalog/descs/descriptor.go +++ b/pkg/sql/catalog/descs/descriptor.go @@ -611,7 +611,7 @@ func (tc *Collection) getNonVirtualDescriptorID( } lookupStoreCacheID := func() (continueOrHalt, descpb.ID, error) { ni := descpb.NameInfo{ParentID: parentID, ParentSchemaID: parentSchemaID, Name: name} - if _, ok := tc.shadowedNames[ni]; ok { + if tc.isShadowedName(ni) { return continueLookups, descpb.InvalidID, nil } if tc.cr.IsNameInCache(&ni) { @@ -645,7 +645,7 @@ func (tc *Collection) getNonVirtualDescriptorID( return haltLookups, descpb.InvalidID, nil } ni := descpb.NameInfo{ParentID: parentID, ParentSchemaID: parentSchemaID, Name: name} - if _, ok := tc.shadowedNames[ni]; ok { + if tc.isShadowedName(ni) { return haltLookups, descpb.InvalidID, nil } read, err := tc.cr.GetByNames(ctx, txn, []descpb.NameInfo{ni}) diff --git a/pkg/sql/logictest/testdata/logic_test/rename_atomic b/pkg/sql/logictest/testdata/logic_test/rename_atomic index e714727bb406..dbc5902adf4e 100644 --- a/pkg/sql/logictest/testdata/logic_test/rename_atomic +++ b/pkg/sql/logictest/testdata/logic_test/rename_atomic @@ -140,3 +140,17 @@ BEGIN; DROP VIEW v; CREATE VIEW v AS SELECT 1; COMMIT + +# Check for correct caching behavior when scanning the namespace table. +subtest regression_93002 + +statement ok +CREATE SCHEMA sc93002 + +statement ok +BEGIN; +SHOW TABLES FROM sc93002; +CREATE TABLE sc93002.t(a INT); +DROP SCHEMA sc93002 CASCADE; +COMMIT; +