Merge cockroachdb#108347

108347: catalog: add more detail to DescriptorNotFoundErrors r=rafiss a=rafiss

This should make it easier to debug some tests where this error is happening rarely

informs cockroachdb#104164

---

### catalog: use NewReferencedDescriptorNotFoundError

This is easier to remember than having to wrap the error at each call
site.

---

### catalog: add more detail to ErrDescriptorNotFound errors

Now the ID will always be shown when this error happens.

---

Release note: None

Co-authored-by: Rafi Shamim <[email protected]>

Author: craig[bot]

pkg/ccl/kvccl/kvfollowerreadsccl/testdata/boundedstaleness/single_row

@@ -244,7 +244,7 @@ ALTER TABLE t2 ADD COLUMN new_col INT
 query idx=2
 SELECT * FROM t2 AS OF SYSTEM TIME with_min_timestamp(now() - '10s', true) WHERE pk = 2
 ----
-pq: referenced descriptor ID 105: descriptor not found
+pq: referenced descriptor ID 105: looking up ID 105: descriptor not found
 events (7 found):
  * event 1: colbatchscan trace on node_idx 2: local read
  * event 2: transaction retry on node_idx: 2
@@ -257,7 +257,7 @@ events (7 found):
 query idx=2
 SELECT * FROM t2 AS OF SYSTEM TIME with_min_timestamp(now() - '10s', true) WHERE pk = 2
 ----
-pq: referenced descriptor ID 105: descriptor not found
+pq: referenced descriptor ID 105: looking up ID 105: descriptor not found
 events (7 found):
  * event 1: colbatchscan trace on node_idx 2: local read
  * event 2: transaction retry on node_idx: 2

pkg/cli/testdata/doctor/test_examine_zipdir

@@ -9,7 +9,7 @@ Examining 37 descriptors and 42 namespace entries...
   ParentID  52, ParentSchemaID 29: relation "vehicle_location_histories" (56): referenced database ID 52: referenced descriptor not found
   ParentID  52, ParentSchemaID 29: relation "promo_codes" (57): referenced database ID 52: referenced descriptor not found
   ParentID  52, ParentSchemaID 29: relation "user_promo_codes" (58): referenced database ID 52: referenced descriptor not found
-  ParentID   0, ParentSchemaID  0: namespace entry "movr" (52): referenced descriptor not found
+  ParentID   0, ParentSchemaID  0: namespace entry "movr" (52): referenced schema ID 52: referenced descriptor not found
 Examining 2 jobs...
 job 587337426984566785: running schema change GC refers to missing table descriptor(s) [59]; existing descriptors that still need to be dropped []; job safe to delete: true.
 ERROR: validation failed

pkg/cli/testdata/doctor/test_examine_zipdir_verbose

@@ -50,7 +50,7 @@ Examining 37 descriptors and 42 namespace entries...
   ParentID  52, ParentSchemaID 29: relation "user_promo_codes" (58): referenced database ID 52: referenced descriptor not found
   ParentID  52, ParentSchemaID 29: relation "user_promo_codes" (58): processed
   ParentID   0, ParentSchemaID  0: namespace entry "defaultdb" (50): processed
-  ParentID   0, ParentSchemaID  0: namespace entry "movr" (52): referenced descriptor not found
+  ParentID   0, ParentSchemaID  0: namespace entry "movr" (52): referenced schema ID 52: referenced descriptor not found
   ParentID   0, ParentSchemaID  0: namespace entry "postgres" (51): processed
   ParentID   0, ParentSchemaID  0: namespace entry "system" (1): processed
   ParentID   1, ParentSchemaID  0: namespace entry "public" (29): processed

pkg/sql/catalog/descs/descriptor.go

@@ -201,7 +201,7 @@ func getDescriptorsByID(
 		if flags.layerFilters.withoutStorage {
 			// Some descriptors are still missing and there's nowhere left to get
 			// them from.
-			return catalog.ErrDescriptorNotFound
+			return errors.Wrapf(catalog.ErrDescriptorNotFound, "looking up descriptor(s) %v", readIDs)
 		}
 		const isDescriptorRequired = true
 		read, err := tc.cr.GetByIDs(ctx, txn, readIDs.Ordered(), isDescriptorRequired, catalog.Any)
@@ -235,7 +235,7 @@ func getDescriptorsByID(
 func filterDescriptor(desc catalog.Descriptor, flags getterFlags) error {
 	if expected := flags.descFilters.maybeParentID; expected != descpb.InvalidID {
 		if actual := desc.GetParentID(); actual != descpb.InvalidID && actual != expected {
-			return catalog.ErrDescriptorNotFound
+			return errors.Wrapf(catalog.ErrDescriptorNotFound, "expected %d, got %d", expected, actual)
 		}
 	}
 	if flags.descFilters.withoutDropped {
@@ -371,7 +371,7 @@ func (q *byIDLookupContext) lookupLeased(
 	// If we have already read all of the descriptors, use it as a negative
 	// cache to short-circuit a lookup we know will be doomed to fail.
 	if q.tc.cr.IsDescIDKnownToNotExist(id, q.flags.descFilters.maybeParentID) {
-		return nil, catalog.NoValidation, catalog.ErrDescriptorNotFound
+		return nil, catalog.NoValidation, catalog.NewDescriptorNotFoundError(id)
 	}
 	desc, shouldReadFromStore, err := q.tc.leased.getByID(q.ctx, q.tc.deadlineHolder(q.txn), id)
 	if err != nil || shouldReadFromStore {

pkg/sql/catalog/descs/hydrate.go

@@ -190,7 +190,7 @@ func HydrateCatalog(ctx context.Context, c nstree.MutableCatalog) error {
 	defer sp.Finish()
 
 	fakeLookupFunc := func(_ context.Context, id descpb.ID, skipHydration bool) (catalog.Descriptor, error) {
-		return nil, catalog.WrapDescRefErr(id, catalog.ErrDescriptorNotFound)
+		return nil, catalog.NewDescriptorNotFoundError(id)
 	}
 	typeLookupFunc := makeTypeLookupFuncForHydration(c, fakeLookupFunc)
 	var hydratable []catalog.Descriptor

pkg/sql/catalog/errors.go

@@ -16,6 +16,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/errors"
+	"github.com/cockroachdb/redact"
 )
 
 // ValidateName validates a name.
@@ -89,10 +90,18 @@ func HasInactiveDescriptorError(err error) bool {
 // found with the given id.
 var ErrDescriptorNotFound = errors.New("descriptor not found")
 
+func NewDescriptorNotFoundError(id descpb.ID) error {
+	return errors.Wrapf(ErrDescriptorNotFound, "looking up ID %d", errors.Safe(id))
+}
+
 // ErrReferencedDescriptorNotFound is like ErrDescriptorNotFound but for
 // descriptors referenced within another descriptor.
 var ErrReferencedDescriptorNotFound = errors.New("referenced descriptor not found")
 
+func NewReferencedDescriptorNotFoundError(descType string, id descpb.ID) error {
+	return errors.Wrapf(ErrReferencedDescriptorNotFound, "referenced %s ID %d", redact.SafeString(descType), errors.Safe(id))
+}
+
 // ErrDescriptorWrongType is returned to signal that a descriptor was found but
 // that it wasn't of the expected type.
 var ErrDescriptorWrongType = errors.New("unexpected descriptor type")

pkg/sql/catalog/internal/catkv/catalog_query.go

@@ -214,5 +214,5 @@ func requiredError(expectedType catalog.DescriptorType, id descpb.ID) (err error
 	default:
 		err = errors.Errorf("failed to find descriptor [%d]", id)
 	}
-	return errors.CombineErrors(catalog.ErrDescriptorNotFound, err)
+	return errors.CombineErrors(catalog.NewDescriptorNotFoundError(id), err)
 }

pkg/sql/catalog/internal/validate/validate.go

@@ -294,7 +294,7 @@ var _ catalog.ValidationDescGetter = (*validationDescGetterImpl)(nil)
 func (vdg *validationDescGetterImpl) GetDescriptor(id descpb.ID) (catalog.Descriptor, error) {
 	desc, found := vdg.descriptors[id]
 	if !found || desc == nil {
-		return nil, catalog.WrapDescRefErr(id, catalog.ErrReferencedDescriptorNotFound)
+		return nil, catalog.NewReferencedDescriptorNotFoundError("descriptor", id)
 	}
 	return desc, nil
 }
@@ -305,7 +305,7 @@ func (vdg *validationDescGetterImpl) GetDatabaseDescriptor(
 ) (catalog.DatabaseDescriptor, error) {
 	desc, found := vdg.descriptors[id]
 	if !found || desc == nil {
-		return nil, catalog.WrapDatabaseDescRefErr(id, catalog.ErrReferencedDescriptorNotFound)
+		return nil, catalog.NewReferencedDescriptorNotFoundError("database", id)
 	}
 	return catalog.AsDatabaseDescriptor(desc)
 }
@@ -316,7 +316,7 @@ func (vdg *validationDescGetterImpl) GetSchemaDescriptor(
 ) (catalog.SchemaDescriptor, error) {
 	desc, found := vdg.descriptors[id]
 	if !found || desc == nil {
-		return nil, catalog.WrapSchemaDescRefErr(id, catalog.ErrReferencedDescriptorNotFound)
+		return nil, catalog.NewReferencedDescriptorNotFoundError("schema", id)
 	}
 	return catalog.AsSchemaDescriptor(desc)
 }
@@ -327,7 +327,7 @@ func (vdg *validationDescGetterImpl) GetTableDescriptor(
 ) (catalog.TableDescriptor, error) {
 	desc, found := vdg.descriptors[id]
 	if !found || desc == nil {
-		return nil, catalog.WrapTableDescRefErr(id, catalog.ErrReferencedDescriptorNotFound)
+		return nil, catalog.NewReferencedDescriptorNotFoundError("table", id)
 	}
 	return catalog.AsTableDescriptor(desc)
 }
@@ -338,7 +338,7 @@ func (vdg *validationDescGetterImpl) GetTypeDescriptor(
 ) (catalog.TypeDescriptor, error) {
 	desc, found := vdg.descriptors[id]
 	if !found || desc == nil {
-		return nil, catalog.WrapTypeDescRefErr(id, catalog.ErrReferencedDescriptorNotFound)
+		return nil, catalog.NewReferencedDescriptorNotFoundError("type", id)
 	}
 	descriptor, err := catalog.AsTypeDescriptor(desc)
 	if err != nil {
@@ -352,7 +352,7 @@ func (vdg *validationDescGetterImpl) GetFunctionDescriptor(
 ) (catalog.FunctionDescriptor, error) {
 	desc, found := vdg.descriptors[id]
 	if !found || desc == nil {
-		return nil, catalog.WrapFunctionDescRefErr(id, catalog.ErrReferencedDescriptorNotFound)
+		return nil, catalog.NewReferencedDescriptorNotFoundError("function", id)
 	}
 	return catalog.AsFunctionDescriptor(desc)
 }

pkg/sql/catalog/lease/lease.go

@@ -933,7 +933,7 @@ func (m *Manager) AcquireByName(
 			// If the name we had doesn't match the newest descriptor in the DB, then
 			// we're trying to use an old name.
 			desc.Release(ctx)
-			return nil, catalog.ErrDescriptorNotFound
+			return nil, catalog.NewDescriptorNotFoundError(id)
 		}
 	}
 	return validateDescriptorForReturn(desc)
@@ -974,7 +974,10 @@ func (m *Manager) resolveName(
 		return id, err
 	}
 	if id == descpb.InvalidID {
-		return id, catalog.ErrDescriptorNotFound
+		return id, errors.Wrapf(catalog.ErrDescriptorNotFound,
+			"resolving name %s with parentID %d and parentSchemaID %d",
+			name, parentID, parentSchemaID,
+		)
 	}
 	return id, nil
 }

pkg/sql/catalog/nstree/catalog.go

@@ -283,7 +283,7 @@ func (c Catalog) ValidateNamespaceEntry(key catalog.NameKey) error {
 	// Compare the namespace entry with the referenced descriptor.
 	desc := c.LookupDescriptor(ne.GetID())
 	if desc == nil {
-		return catalog.ErrReferencedDescriptorNotFound
+		return catalog.NewReferencedDescriptorNotFoundError("schema", ne.GetID())
 	}
 	if desc.Dropped() {
 		return catalog.ErrDescriptorDropped