Merge #93813

93813: descs: add ByIDGetter and ByNameGetter interfaces r=postamar a=postamar

These pave the way for deprecating the existing `Get*ByID` and `Get*ByName` `Collection` methods.

Informs #87753.

Release note: None

Co-authored-by: Marius Posta <[email protected]>

pkg/ccl/backupccl/backupresolver/targets.go

@@ -377,12 +377,9 @@ func DescriptorsMatchingTargets(
 		if _, ok := alreadyRequestedDBs[dbID]; !ok {
 			desc := r.DescByID[dbID]
 			// Verify that the database is in the correct state.
-			doesNotExistErr := errors.Errorf(`database %q does not exist`, d)
-			if err := catalog.FilterDescriptorState(
-				desc, tree.CommonLookupFlags{},
-			); err != nil {
+			if desc == nil || !desc.Public() {
 				// Return a does not exist error if explicitly asking for this database.
-				return ret, doesNotExistErr
+				return ret, errors.Errorf(`database %q does not exist`, d)
 			}
 			ret.Descs = append(ret.Descs, desc)
 			ret.RequestedDBs = append(ret.RequestedDBs, desc.(catalog.DatabaseDescriptor))
@@ -401,15 +398,14 @@ func DescriptorsMatchingTargets(
 		}
 		if _, ok := alreadyRequestedSchemas[id]; !ok {
 			schemaDesc := r.DescByID[id]
-			if err := catalog.FilterDescriptorState(
-				schemaDesc, tree.CommonLookupFlags{IncludeOffline: !requirePublic},
-			); err != nil {
+			if schemaDesc == nil || !schemaDesc.Public() {
 				if requirePublic {
 					return errors.Wrapf(err, "schema %d was expected to be PUBLIC", id)
+				} else if schemaDesc == nil || !schemaDesc.Offline() {
+					// If the schema is not public, but we don't require it to be, ignore
+					// it.
+					return nil
 				}
-				// If the schema is not public, but we don't require it to be, ignore
-				// it.
-				return nil
 			}
 			alreadyRequestedSchemas[id] = struct{}{}
 			ret.Descs = append(ret.Descs, r.DescByID[id])
@@ -485,16 +481,8 @@ func DescriptorsMatchingTargets(
 			}
 			tableDesc, isTable := descI.(catalog.TableDescriptor)
 			// If the type assertion didn't work, then we resolved a type instead, so
-			// error out.
-			if !isTable {
-				return ret, doesNotExistErr
-			}
-
-			// Verify that the table is in the correct state.
-			if err := catalog.FilterDescriptorState(
-				tableDesc, tree.CommonLookupFlags{},
-			); err != nil {
-				// Return a does not exist error if explicitly asking for this table.
+			// error out. Otherwise verify that the table is in the correct state.
+			if !isTable || tableDesc == nil || !tableDesc.Public() {
 				return ret, doesNotExistErr
 			}
 
@@ -591,9 +579,7 @@ func DescriptorsMatchingTargets(
 	addObjectDescsInSchema := func(objectsIDs *catalog.DescriptorIDSet) error {
 		for _, id := range objectsIDs.Ordered() {
 			desc := r.DescByID[id]
-			if err := catalog.FilterDescriptorState(
-				desc, tree.CommonLookupFlags{IncludeOffline: true},
-			); err != nil {
+			if desc == nil || (!desc.Public() && !desc.Offline()) {
 				// Don't include this object in the expansion since it's not in a valid
 				// state. Silently fail since this object was not directly requested,
 				// but was just part of an expansion.

pkg/sql/catalog/catalog.go

@@ -67,7 +67,7 @@ type VirtualSchema interface {
 	Desc() SchemaDescriptor
 	NumTables() int
 	VisitTables(func(object VirtualObject))
-	GetObjectByName(name string, flags tree.ObjectLookupFlags) (VirtualObject, error)
+	GetObjectByName(name string, kind tree.DesiredObjectKind) (VirtualObject, error)
 }
 
 // VirtualObject is a virtual schema object.

pkg/sql/catalog/descriptor.go

@@ -928,46 +928,38 @@ type FunctionDescriptor interface {
 	ToCreateExpr() (*tree.CreateFunction, error)
 }
 
-// FilterDescriptorState inspects the state of a given descriptor and returns an
-// error if the state is anything but public. The error describes the state of
-// the descriptor.
-func FilterDescriptorState(desc Descriptor, flags tree.CommonLookupFlags) error {
-	if flags.ParentID != 0 {
-		parent := desc.GetParentID()
-		if parent != 0 && parent != flags.ParentID {
-			return ErrDescriptorNotFound
-		}
+// FilterDroppedDescriptor returns an error if the descriptor state is DROP.
+func FilterDroppedDescriptor(desc Descriptor) error {
+	if !desc.Dropped() {
+		return nil
+
 	}
+	return NewInactiveDescriptorError(ErrDescriptorDropped)
+}
 
-	switch {
-	case desc.Dropped() && !flags.IncludeDropped:
-		return NewInactiveDescriptorError(ErrDescriptorDropped)
-	case desc.Offline() && !flags.IncludeOffline:
-		err := errors.Errorf("%s %q is offline", desc.DescriptorType(), desc.GetName())
-		if desc.GetOfflineReason() != "" {
-			err = errors.Errorf("%s %q is offline: %s", desc.DescriptorType(), desc.GetName(), desc.GetOfflineReason())
-		}
-		return NewInactiveDescriptorError(err)
-	case desc.Adding() &&
-		// The ADD state is special.
-		// We don't want adding descriptors to be visible to DML queries, but we
-		// want them to be visible to schema changes:
-		//   - when uncommitted we want them to be accessible by name for other
-		//     schema changes, e.g.
-		//       BEGIN; CREATE TABLE t ... ; ALTER TABLE t RENAME TO ...;
-		//     should be possible.
-		//   - when committed we want them to be accessible to their own schema
-		//     change job, where they're referenced by ID.
-		//
-		// The AvoidCommittedAdding is set if and only if the lookup is by-name
-		// and prevents them from seeing committed adding descriptors.
-		!(flags.AvoidCommittedAdding && desc.IsUncommittedVersion() && (flags.AvoidLeased || flags.RequireMutable)) &&
-		!(!flags.AvoidCommittedAdding && (desc.IsUncommittedVersion() || flags.AvoidLeased || flags.RequireMutable)):
-		// For the time being, only table descriptors can be in the adding state.
-		return pgerror.WithCandidateCode(newAddingTableError(desc.(TableDescriptor)),
-			pgcode.ObjectNotInPrerequisiteState)
+// FilterOfflineDescriptor returns an error if the descriptor state is OFFLINE.
+func FilterOfflineDescriptor(desc Descriptor) error {
+	if !desc.Offline() {
+		return nil
+	}
+	err := errors.Errorf("%s %q is offline", desc.DescriptorType(), desc.GetName())
+	if desc.GetOfflineReason() != "" {
+		err = errors.Errorf("%s %q is offline: %s", desc.DescriptorType(), desc.GetName(), desc.GetOfflineReason())
+	}
+	return NewInactiveDescriptorError(err)
+}
+
+// FilterAddingDescriptor returns an error if the descriptor state is ADD.
+func FilterAddingDescriptor(desc Descriptor) error {
+	if !desc.Adding() {
+		return nil
+	}
+	// For the time being, only table descriptors can be in the adding state.
+	tbl, err := AsTableDescriptor(desc)
+	if err != nil {
+		return errors.HandleAsAssertionFailure(err)
 	}
-	return nil
+	return pgerror.WithCandidateCode(newAddingTableError(tbl), pgcode.ObjectNotInPrerequisiteState)
 }
 
 // TableLookupFn is used to resolve a table from an ID, particularly when

pkg/sql/catalog/descs/BUILD.bazel

@@ -11,16 +11,15 @@ go_library(
         "errors.go",
         "factory.go",
         "function.go",
+        "getters.go",
         "helpers.go",
         "hydrate.go",
         "leased_descriptors.go",
-        "object.go",
         "schema.go",
         "session_data.go",
         "synthetic_descriptors.go",
         "system_table.go",
         "table.go",
-        "table_name.go",
         "temporary_descriptors.go",
         "txn.go",
         "type.go",

pkg/sql/catalog/descs/collection.go

@@ -585,12 +585,9 @@ func (tc *Collection) lookupDescriptorID(
 	// First look up in-memory descriptors in collection,
 	// except for leased descriptors.
 	objInMemory, err := func() (catalog.Descriptor, error) {
-		flags := tree.CommonLookupFlags{
-			Required:       true,
-			AvoidLeased:    true,
-			AvoidStorage:   true,
-			IncludeOffline: true,
-		}
+		flags := defaultUnleasedFlags()
+		flags.layerFilters.withoutStorage = true
+		flags.descFilters.withoutDropped = true
 		var db catalog.DatabaseDescriptor
 		var sc catalog.SchemaDescriptor
 		expectedType := catalog.Database
@@ -618,13 +615,9 @@ func (tc *Collection) lookupDescriptorID(
 				}
 			}
 		}
-		return tc.getDescriptorByName(ctx, txn, db, sc, key.Name, flags, expectedType)
+		return getDescriptorByName(ctx, txn, tc, db, sc, key.Name, flags, expectedType)
 	}()
-	if errors.IsAny(err, catalog.ErrDescriptorNotFound, catalog.ErrDescriptorDropped) {
-		// Swallow these errors to fall back to storage lookup.
-		err = nil
-	}
-	if err != nil {
+	if err != nil && !errors.Is(err, catalog.ErrDescriptorNotFound) {
 		return descpb.InvalidID, err
 	}
 	if objInMemory != nil {
@@ -978,13 +971,10 @@ func (tc *Collection) aggregateAllLayers(
 	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,
-	}
-	allDescs, err := tc.getDescriptorsByID(ctx, txn, flags, descIDs.Ordered()...)
-	if err != nil {
+	allDescs := make([]catalog.Descriptor, descIDs.Len())
+	if err := getDescriptorsByID(
+		ctx, tc, txn, defaultUnleasedFlags(), allDescs, descIDs.Ordered()...,
+	); err != nil {
 		return nstree.MutableCatalog{}, err
 	}
 	for _, desc := range allDescs {
@@ -1002,15 +992,12 @@ 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 nstree.Catalog{}, err
+	{
+		var err error
+		db, err = ByIDGetter(makeGetterBase(txn, tc, defaultUnleasedFlags())).Database(ctx, db.GetID())
+		if err != nil {
+			return nstree.Catalog{}, err
+		}
 	}
 	c, err := tc.GetAllInDatabase(ctx, txn, db)
 	if err != nil {

pkg/sql/catalog/descs/database.go

@@ -12,89 +12,35 @@ package descs
 
 import (
 	"context"
-	"fmt"
 
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
-	"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
-	"github.com/cockroachdb/errors"
 )
 
 // GetMutableDatabaseByName returns a mutable database descriptor with
 // properties according to the provided lookup flags. RequireMutable is ignored.
 func (tc *Collection) GetMutableDatabaseByName(
 	ctx context.Context, txn *kv.Txn, name string, flags tree.DatabaseLookupFlags,
 ) (*dbdesc.Mutable, error) {
-	flags.RequireMutable = true
-	desc, err := tc.getDatabaseByName(ctx, txn, name, flags)
-	if err != nil || desc == nil {
-		return nil, err
-	}
-	return desc.(*dbdesc.Mutable), nil
+	return tc.ByName(txn).WithFlags(flags).Mutable().Database(ctx, name)
 }
 
 // GetImmutableDatabaseByName returns an immutable database descriptor with
 // properties according to the provided lookup flags. RequireMutable is ignored.
 func (tc *Collection) GetImmutableDatabaseByName(
 	ctx context.Context, txn *kv.Txn, name string, flags tree.DatabaseLookupFlags,
 ) (catalog.DatabaseDescriptor, error) {
-	flags.RequireMutable = false
-	return tc.getDatabaseByName(ctx, txn, name, flags)
-}
-
-// getDatabaseByName returns a database descriptor with properties according to
-// the provided lookup flags.
-func (tc *Collection) getDatabaseByName(
-	ctx context.Context, txn *kv.Txn, name string, flags tree.DatabaseLookupFlags,
-) (catalog.DatabaseDescriptor, error) {
-	desc, err := tc.getDescriptorByName(ctx, txn, nil /* db */, nil /* sc */, name, flags, catalog.Database)
-	if err != nil {
-		return nil, err
-	}
-	if desc == nil {
-		if flags.Required {
-			return nil, sqlerrors.NewUndefinedDatabaseError(name)
-		}
-		return nil, nil
-	}
-	db, ok := desc.(catalog.DatabaseDescriptor)
-	if !ok {
-		if flags.Required {
-			return nil, sqlerrors.NewUndefinedDatabaseError(name)
-		}
-		return nil, nil
-	}
-	return db, nil
+	return tc.ByName(txn).WithFlags(flags).Immutable().Database(ctx, name)
 }
 
 // GetImmutableDatabaseByID returns an immutable database descriptor with
 // properties according to the provided lookup flags. RequireMutable is ignored.
 func (tc *Collection) GetImmutableDatabaseByID(
 	ctx context.Context, txn *kv.Txn, dbID descpb.ID, flags tree.DatabaseLookupFlags,
 ) (bool, catalog.DatabaseDescriptor, error) {
-	flags.RequireMutable = false
-	return tc.getDatabaseByID(ctx, txn, dbID, flags)
-}
-
-func (tc *Collection) getDatabaseByID(
-	ctx context.Context, txn *kv.Txn, dbID descpb.ID, flags tree.DatabaseLookupFlags,
-) (bool, catalog.DatabaseDescriptor, error) {
-	descs, err := tc.getDescriptorsByID(ctx, txn, flags, dbID)
-	if err != nil {
-		if errors.Is(err, catalog.ErrDescriptorNotFound) {
-			if flags.Required {
-				return false, nil, sqlerrors.NewUndefinedDatabaseError(fmt.Sprintf("[%d]", dbID))
-			}
-			return false, nil, nil
-		}
-		return false, nil, err
-	}
-	db, ok := descs[0].(catalog.DatabaseDescriptor)
-	if !ok {
-		return false, nil, sqlerrors.NewUndefinedDatabaseError(fmt.Sprintf("[%d]", dbID))
-	}
-	return true, db, nil
+	db, err := tc.ByID(txn).WithFlags(flags).Immutable().Database(ctx, dbID)
+	return db != nil, db, err
 }