From 15977ba62dba23b66f6a24eb9fa30e56a1615bb4 Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Tue, 10 Jan 2023 15:24:31 -0500 Subject: [PATCH 1/9] typedesc: change GetIDClosure method signature This commit refactors unnecessary errors away. Release note: None --- pkg/sql/catalog/descriptor.go | 2 +- pkg/sql/catalog/tabledesc/structured.go | 56 ++++------------- pkg/sql/catalog/tabledesc/validate.go | 8 +-- .../typedesc/table_implicit_record_type.go | 5 +- pkg/sql/catalog/typedesc/type_desc.go | 63 +++++-------------- pkg/sql/drop_type.go | 22 ++----- pkg/sql/opt/optbuilder/builder.go | 9 +-- pkg/sql/opt/optbuilder/create_function.go | 21 +++---- pkg/sql/schema_changer.go | 7 +-- .../schemachanger/scbuild/builder_state.go | 18 +----- pkg/sql/schemachanger/scdecomp/decomp.go | 19 ++---- pkg/sql/schemachanger/scdecomp/helpers.go | 24 ++----- .../scexec/scmutationexec/references.go | 8 +-- 13 files changed, 62 insertions(+), 200 deletions(-) diff --git a/pkg/sql/catalog/descriptor.go b/pkg/sql/catalog/descriptor.go index 4ee814d2d588..5ceb28dfbf2c 100644 --- a/pkg/sql/catalog/descriptor.go +++ b/pkg/sql/catalog/descriptor.go @@ -779,7 +779,7 @@ type TypeDescriptor interface { HasPendingSchemaChanges() bool // GetIDClosure returns all type descriptor IDs that are referenced by this // type descriptor. - GetIDClosure() (map[descpb.ID]struct{}, error) + GetIDClosure() DescriptorIDSet // IsCompatibleWith returns whether the type "desc" is compatible with "other". // As of now "compatibility" entails that disk encoded data of "desc" can be // interpreted and used by "other". diff --git a/pkg/sql/catalog/tabledesc/structured.go b/pkg/sql/catalog/tabledesc/structured.go index 23778d0ebf29..075f2e992b71 100644 --- a/pkg/sql/catalog/tabledesc/structured.go +++ b/pkg/sql/catalog/tabledesc/structured.go @@ -326,11 +326,7 @@ func (desc *wrapper) GetAllReferencedTypeIDs( if err != nil { return nil, nil, err } - referencedInColumns = make(descpb.IDs, 0, len(ids)) - for id := range ids { - referencedInColumns = append(referencedInColumns, id) - } - sort.Sort(referencedInColumns) + referencedInColumns = ids.Ordered() // REGIONAL BY TABLE tables may have a dependency with the multi-region enum. exists := desc.GetMultiRegionEnumDependencyIfExists() @@ -339,24 +335,16 @@ func (desc *wrapper) GetAllReferencedTypeIDs( if err != nil { return nil, nil, err } - ids[regionEnumID] = struct{}{} + ids.Add(regionEnumID) } // Add any other type dependencies that are not // used in a column (specifically for views). for _, id := range desc.DependsOnTypes { - ids[id] = struct{}{} - } - - // Construct the output. - result := make(descpb.IDs, 0, len(ids)) - for id := range ids { - result = append(result, id) + ids.Add(id) } - // Sort the output so that the order is deterministic. - sort.Sort(result) - return result, referencedInColumns, nil + return ids.Ordered(), referencedInColumns, nil } // getAllReferencedTypesInTableColumns returns a map of all user defined @@ -371,7 +359,7 @@ func (desc *wrapper) GetAllReferencedTypeIDs( // GetAllReferencedTypesByID accounts for this dependency. func (desc *wrapper) getAllReferencedTypesInTableColumns( getType func(descpb.ID) (catalog.TypeDescriptor, error), -) (map[descpb.ID]struct{}, error) { +) (ret catalog.DescriptorIDSet, _ error) { // All serialized expressions within a table descriptor are serialized // with type annotations as ID's, so this visitor will collect them all. visitor := &tree.TypeCollectorVisitor{ @@ -388,52 +376,34 @@ func (desc *wrapper) getAllReferencedTypesInTableColumns( } if err := ForEachExprStringInTableDesc(desc, addOIDsInExpr); err != nil { - return nil, err + return ret, err } // For each of the collected type IDs in the table descriptor expressions, // collect the closure of IDs referenced. - ids := make(map[descpb.ID]struct{}) for id := range visitor.OIDs { uid := typedesc.UserDefinedTypeOIDToID(id) typDesc, err := getType(uid) if err != nil { - return nil, err - } - children, err := typDesc.GetIDClosure() - if err != nil { - return nil, err - } - for child := range children { - ids[child] = struct{}{} + return ret, err } + typDesc.GetIDClosure().ForEach(ret.Add) } // Now add all of the column types in the table. - addIDsInColumn := func(c *descpb.ColumnDescriptor) error { - children, err := typedesc.GetTypeDescriptorClosure(c.Type) - if err != nil { - return err - } - for id := range children { - ids[id] = struct{}{} - } - return nil + addIDsInColumn := func(c *descpb.ColumnDescriptor) { + typedesc.GetTypeDescriptorClosure(c.Type).ForEach(ret.Add) } for i := range desc.Columns { - if err := addIDsInColumn(&desc.Columns[i]); err != nil { - return nil, err - } + addIDsInColumn(&desc.Columns[i]) } for _, mut := range desc.Mutations { if c := mut.GetColumn(); c != nil { - if err := addIDsInColumn(c); err != nil { - return nil, err - } + addIDsInColumn(c) } } - return ids, nil + return ret, nil } func (desc *Mutable) initIDs() { diff --git a/pkg/sql/catalog/tabledesc/validate.go b/pkg/sql/catalog/tabledesc/validate.go index 1c47b7cf8ee6..91d8ab4c1ec6 100644 --- a/pkg/sql/catalog/tabledesc/validate.go +++ b/pkg/sql/catalog/tabledesc/validate.go @@ -76,13 +76,7 @@ func (desc *wrapper) GetReferencedDescIDs() (catalog.DescriptorIDSet, error) { } // Collect user defined type Oids and sequence references in columns. for _, col := range desc.DeletableColumns() { - children, err := typedesc.GetTypeDescriptorClosure(col.GetType()) - if err != nil { - return catalog.DescriptorIDSet{}, err - } - for id := range children { - ids.Add(id) - } + typedesc.GetTypeDescriptorClosure(col.GetType()).ForEach(ids.Add) for i := 0; i < col.NumUsesSequences(); i++ { ids.Add(col.GetUsesSequenceID(i)) } diff --git a/pkg/sql/catalog/typedesc/table_implicit_record_type.go b/pkg/sql/catalog/typedesc/table_implicit_record_type.go index a90379dfd8e3..2af832fcdd51 100644 --- a/pkg/sql/catalog/typedesc/table_implicit_record_type.go +++ b/pkg/sql/catalog/typedesc/table_implicit_record_type.go @@ -233,8 +233,9 @@ func (v TableImplicitRecordType) AsTypesT() *types.T { func (v TableImplicitRecordType) HasPendingSchemaChanges() bool { return false } // GetIDClosure implements the TypeDescriptor interface. -func (v TableImplicitRecordType) GetIDClosure() (map[descpb.ID]struct{}, error) { - return nil, errors.AssertionFailedf("IDClosure unsupported for implicit table record types") +func (v TableImplicitRecordType) GetIDClosure() catalog.DescriptorIDSet { + v.panicNotSupported("GetIDClosure") + return catalog.DescriptorIDSet{} } // IsCompatibleWith implements the TypeDescriptorInterface. diff --git a/pkg/sql/catalog/typedesc/type_desc.go b/pkg/sql/catalog/typedesc/type_desc.go index d7b7d21c49ac..ddce6be08dc9 100644 --- a/pkg/sql/catalog/typedesc/type_desc.go +++ b/pkg/sql/catalog/typedesc/type_desc.go @@ -627,13 +627,7 @@ func (desc *immutable) GetReferencedDescIDs() (catalog.DescriptorIDSet, error) { if desc.GetParentSchemaID() != keys.PublicSchemaID { ids.Add(desc.GetParentSchemaID()) } - children, err := desc.GetIDClosure() - if err != nil { - return catalog.DescriptorIDSet{}, err - } - for id := range children { - ids.Add(id) - } + desc.GetIDClosure().ForEach(ids.Add) return ids, nil } @@ -980,36 +974,23 @@ func (desc *immutable) ForEachUDTDependentForHydration(fn func(t *types.T) error } // GetIDClosure implements the TypeDescriptor interface. -func (desc *immutable) GetIDClosure() (map[descpb.ID]struct{}, error) { - ret := make(map[descpb.ID]struct{}) +func (desc *immutable) GetIDClosure() (ret catalog.DescriptorIDSet) { // Collect the descriptor's own ID. - ret[desc.ID] = struct{}{} + ret.Add(desc.ID) switch desc.Kind { case descpb.TypeDescriptor_ALIAS: // If this descriptor is an alias for another type, then get collect the // closure for alias. - children, err := GetTypeDescriptorClosure(desc.Alias) - if err != nil { - return nil, err - } - for id := range children { - ret[id] = struct{}{} - } + GetTypeDescriptorClosure(desc.Alias).ForEach(ret.Add) case descpb.TypeDescriptor_COMPOSITE: for _, e := range desc.Composite.Elements { - children, err := GetTypeDescriptorClosure(e.ElementType) - if err != nil { - return nil, err - } - for id := range children { - ret[id] = struct{}{} - } + GetTypeDescriptorClosure(e.ElementType).ForEach(ret.Add) } default: // Otherwise, take the array type ID. - ret[desc.ArrayTypeID] = struct{}{} + ret.Add(desc.ArrayTypeID) } - return ret, nil + return ret } // GetObjectType implements the Object interface. @@ -1019,42 +1000,26 @@ func (desc *immutable) GetObjectType() privilege.ObjectType { // GetTypeDescriptorClosure returns all type descriptor IDs that are // referenced by this input types.T. -func GetTypeDescriptorClosure(typ *types.T) (map[descpb.ID]struct{}, error) { +func GetTypeDescriptorClosure(typ *types.T) (ret catalog.DescriptorIDSet) { if !typ.UserDefined() { - return map[descpb.ID]struct{}{}, nil + return catalog.DescriptorIDSet{} } - id := GetUserDefinedTypeDescID(typ) // Collect the type's descriptor ID. - ret := map[descpb.ID]struct{}{ - id: {}, - } + ret.Add(GetUserDefinedTypeDescID(typ)) switch typ.Family() { case types.ArrayFamily: // If we have an array type, then collect all types in the contents. - children, err := GetTypeDescriptorClosure(typ.ArrayContents()) - if err != nil { - return nil, err - } - for id := range children { - ret[id] = struct{}{} - } + GetTypeDescriptorClosure(typ.ArrayContents()).ForEach(ret.Add) case types.TupleFamily: // If we have a tuple type, collect all types in the contents. for _, elt := range typ.TupleContents() { - children, err := GetTypeDescriptorClosure(elt) - if err != nil { - return nil, err - } - for id := range children { - ret[id] = struct{}{} - } + GetTypeDescriptorClosure(elt).ForEach(ret.Add) } default: // Otherwise, take the array type ID. - id := GetUserDefinedArrayTypeDescID(typ) - ret[id] = struct{}{} + ret.Add(GetUserDefinedArrayTypeDescID(typ)) } - return ret, nil + return ret } // SetDeclarativeSchemaChangerState is part of the catalog.MutableDescriptor diff --git a/pkg/sql/drop_type.go b/pkg/sql/drop_type.go index 95f696b08bc2..32d101a27627 100644 --- a/pkg/sql/drop_type.go +++ b/pkg/sql/drop_type.go @@ -242,11 +242,8 @@ func (p *planner) removeBackRefsFromAllTypesInTable( } func (p *planner) addBackRefsFromAllTypesInType(ctx context.Context, desc *typedesc.Mutable) error { - typeIDs, err := desc.GetIDClosure() - if err != nil { - return err - } - for id := range typeIDs { + typeIDs := desc.GetIDClosure() + for _, id := range typeIDs.Ordered() { if id == desc.ID { // Don't add a self back reference. continue @@ -262,18 +259,9 @@ func (p *planner) addBackRefsFromAllTypesInType(ctx context.Context, desc *typed func (p *planner) removeBackRefsFromAllTypesInType( ctx context.Context, desc *typedesc.Mutable, ) error { - typeIDMap, err := desc.GetIDClosure() - if err != nil { - return err - } - typeIDs := catalog.DescriptorIDSet{} - for id := range typeIDMap { - if id == desc.ID { - // Don't add a self back reference. - continue - } - typeIDs.Add(id) - } + typeIDs := desc.GetIDClosure() + // Don't add a self back reference. + typeIDs.Remove(desc.ID) jobDesc := fmt.Sprintf("updating type back references %v for type %d", typeIDs, desc.ID) return p.removeTypeBackReferences(ctx, typeIDs.Ordered(), desc.ID, jobDesc) } diff --git a/pkg/sql/opt/optbuilder/builder.go b/pkg/sql/opt/optbuilder/builder.go index e2fddac93902..4acf004a4943 100644 --- a/pkg/sql/opt/optbuilder/builder.go +++ b/pkg/sql/opt/optbuilder/builder.go @@ -14,6 +14,7 @@ import ( "context" "strconv" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" "github.com/cockroachdb/cockroach/pkg/sql/delegate" "github.com/cockroachdb/cockroach/pkg/sql/opt" @@ -479,13 +480,9 @@ func (b *Builder) maybeTrackRegclassDependenciesForViews(texpr tree.TypedExpr) { func (b *Builder) maybeTrackUserDefinedTypeDepsForViews(texpr tree.TypedExpr) { if b.trackSchemaDeps { if texpr.ResolvedType().UserDefined() { - children, err := typedesc.GetTypeDescriptorClosure(texpr.ResolvedType()) - if err != nil { - panic(err) - } - for id := range children { + typedesc.GetTypeDescriptorClosure(texpr.ResolvedType()).ForEach(func(id descpb.ID) { b.schemaTypeDeps.Add(int(id)) - } + }) } } } diff --git a/pkg/sql/opt/optbuilder/create_function.go b/pkg/sql/opt/optbuilder/create_function.go index 0d1418cd7497..0f3149e5fd9b 100644 --- a/pkg/sql/opt/optbuilder/create_function.go +++ b/pkg/sql/opt/optbuilder/create_function.go @@ -11,6 +11,7 @@ package optbuilder import ( + "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" "github.com/cockroachdb/cockroach/pkg/sql/opt" "github.com/cockroachdb/cockroach/pkg/sql/opt/memo" @@ -112,13 +113,9 @@ func (b *Builder) buildCreateFunction(cf *tree.CreateFunction, inScope *scope) ( col.setParamOrd(i) // Collect the user defined type dependencies. - typeIDs, err := typedesc.GetTypeDescriptorClosure(typ) - if err != nil { - panic(err) - } - for typeID := range typeIDs { - typeDeps.Add(int(typeID)) - } + typedesc.GetTypeDescriptorClosure(typ).ForEach(func(id descpb.ID) { + typeDeps.Add(int(id)) + }) } // Collect the user defined type dependency of the return type. @@ -126,13 +123,9 @@ func (b *Builder) buildCreateFunction(cf *tree.CreateFunction, inScope *scope) ( if err != nil { panic(err) } - typeIDs, err := typedesc.GetTypeDescriptorClosure(funcReturnType) - if err != nil { - panic(err) - } - for typeID := range typeIDs { - typeDeps.Add(int(typeID)) - } + typedesc.GetTypeDescriptorClosure(funcReturnType).ForEach(func(id descpb.ID) { + typeDeps.Add(int(id)) + }) // Parse the function body. stmts, err := parser.Parse(funcBodyStr) diff --git a/pkg/sql/schema_changer.go b/pkg/sql/schema_changer.go index 7e974eb84cee..2a74e5cd5e11 100644 --- a/pkg/sql/schema_changer.go +++ b/pkg/sql/schema_changer.go @@ -959,11 +959,8 @@ func (sc *SchemaChanger) dropViewDeps( } // Clean up sequence and type references from the view. for _, col := range viewDesc.DeletableColumns() { - typeClosure, err := typedesc.GetTypeDescriptorClosure(col.GetType()) - if err != nil { - return err - } - for id := range typeClosure { + typeClosure := typedesc.GetTypeDescriptorClosure(col.GetType()) + for _, id := range typeClosure.Ordered() { typeDesc, err := descsCol.MutableByID(txn).Type(ctx, id) if err != nil { log.Warningf(ctx, "error resolving type dependency %d", id) diff --git a/pkg/sql/schemachanger/scbuild/builder_state.go b/pkg/sql/schemachanger/scbuild/builder_state.go index 133420e85ab3..4a63f264619f 100644 --- a/pkg/sql/schemachanger/scbuild/builder_state.go +++ b/pkg/sql/schemachanger/scbuild/builder_state.go @@ -382,15 +382,7 @@ func (b *builderState) ResolveTypeRef(ref tree.ResolvableTypeReference) scpb.Typ } func newTypeT(t *types.T) scpb.TypeT { - m, err := typedesc.GetTypeDescriptorClosure(t) - if err != nil { - panic(err) - } - var ids catalog.DescriptorIDSet - for id := range m { - ids.Add(id) - } - return scpb.TypeT{Type: t, ClosedTypeIDs: ids.Ordered()} + return scpb.TypeT{Type: t, ClosedTypeIDs: typedesc.GetTypeDescriptorClosure(t).Ordered()} } // WrapExpression implements the scbuildstmt.TableHelpers interface. @@ -439,13 +431,7 @@ func (b *builderState) WrapExpression(tableID catid.DescID, expr tree.Expr) *scp if err != nil { panic(err) } - ids, err := typ.GetIDClosure() - if err != nil { - panic(err) - } - for id = range ids { - typeIDs.Add(id) - } + typ.GetIDClosure().ForEach(typeIDs.Add) } } // Collect sequence IDs. diff --git a/pkg/sql/schemachanger/scdecomp/decomp.go b/pkg/sql/schemachanger/scdecomp/decomp.go index 6e48de6a830f..6bb135d3ffd9 100644 --- a/pkg/sql/schemachanger/scdecomp/decomp.go +++ b/pkg/sql/schemachanger/scdecomp/decomp.go @@ -32,7 +32,7 @@ type walkCtx struct { desc catalog.Descriptor ev ElementVisitor lookupFn func(id catid.DescID) catalog.Descriptor - cachedTypeIDClosures map[catid.DescID]map[catid.DescID]struct{} + cachedTypeIDClosures map[catid.DescID]catalog.DescriptorIDSet backRefs catalog.DescriptorIDSet commentReader CommentGetter zoneConfigReader ZoneConfigGetter @@ -70,7 +70,7 @@ func WalkDescriptor( desc: desc, ev: ev, lookupFn: lookupFn, - cachedTypeIDClosures: make(map[catid.DescID]map[catid.DescID]struct{}), + cachedTypeIDClosures: make(map[catid.DescID]catalog.DescriptorIDSet), commentReader: commentReader, zoneConfigReader: zoneConfigReader, } @@ -168,11 +168,7 @@ func (w *walkCtx) walkSchema(sc catalog.SchemaDescriptor) { func (w *walkCtx) walkType(typ catalog.TypeDescriptor) { switch typ.GetKind() { case descpb.TypeDescriptor_ALIAS: - typeT, err := newTypeT(typ.TypeDesc().Alias) - if err != nil { - panic(errors.NewAssertionErrorWithWrappedErrf(err, "alias type %q (%d)", - typ.GetName(), typ.GetID())) - } + typeT := newTypeT(typ.TypeDesc().Alias) w.ev(descriptorStatus(typ), &scpb.AliasType{ TypeID: typ.GetID(), TypeT: *typeT, @@ -197,11 +193,7 @@ func (w *walkCtx) walkType(typ catalog.TypeDescriptor) { }) composite := typ.TypeDesc().Composite for _, e := range composite.Elements { - typeT, err := newTypeT(e.ElementType) - if err != nil { - panic(errors.NewAssertionErrorWithWrappedErrf(err, "alias type %q (%d)", - typ.GetName(), typ.GetID())) - } + typeT := newTypeT(e.ElementType) w.ev(descriptorStatus(typ), &scpb.CompositeTypeAttrType{ CompositeTypeID: typ.GetID(), TypeT: *typeT, @@ -440,8 +432,7 @@ func (w *walkCtx) walkColumn(tbl catalog.TableDescriptor, col catalog.Column) { } return nil }) - typeT, err := newTypeT(col.GetType()) - onErrPanic(err) + typeT := newTypeT(col.GetType()) columnType.TypeT = *typeT if col.IsComputed() { diff --git a/pkg/sql/schemachanger/scdecomp/helpers.go b/pkg/sql/schemachanger/scdecomp/helpers.go index 84503458ea8f..ee0a2a52ac2d 100644 --- a/pkg/sql/schemachanger/scdecomp/helpers.go +++ b/pkg/sql/schemachanger/scdecomp/helpers.go @@ -94,14 +94,9 @@ func (w *walkCtx) newExpression(expr string) (*scpb.Expression, error) { if err != nil { return nil, err } - w.cachedTypeIDClosures[id], err = typ.GetIDClosure() - if err != nil { - return nil, err - } - } - for id = range w.cachedTypeIDClosures[id] { - typIDs.Add(id) + w.cachedTypeIDClosures[id] = typ.GetIDClosure() } + w.cachedTypeIDClosures[id].ForEach(typIDs.Add) } } @@ -119,18 +114,9 @@ func (w *walkCtx) newExpression(expr string) (*scpb.Expression, error) { }, nil } -func newTypeT(t *types.T) (*scpb.TypeT, error) { - ids, err := typedesc.GetTypeDescriptorClosure(t) - if err != nil { - return nil, err - } - var ret catalog.DescriptorIDSet - for id := range ids { - ret.Add(id) - } - ret.Remove(descpb.InvalidID) +func newTypeT(t *types.T) *scpb.TypeT { return &scpb.TypeT{ Type: t, - ClosedTypeIDs: ret.Ordered(), - }, nil + ClosedTypeIDs: typedesc.GetTypeDescriptorClosure(t).Ordered(), + } } diff --git a/pkg/sql/schemachanger/scexec/scmutationexec/references.go b/pkg/sql/schemachanger/scexec/scmutationexec/references.go index e2ab1ba671c2..5f2f34ffed9a 100644 --- a/pkg/sql/schemachanger/scexec/scmutationexec/references.go +++ b/pkg/sql/schemachanger/scexec/scmutationexec/references.go @@ -197,13 +197,7 @@ func (m *visitor) UpdateTypeBackReferencesInTypes( if err != nil { return err } - ids, err := typ.GetIDClosure() - if err != nil { - return err - } - for id := range ids { - forwardRefs.Add(id) - } + forwardRefs = typ.GetIDClosure() } return updateBackReferencesInTypes(ctx, m, op.TypeIDs, op.BackReferencedTypeID, forwardRefs) } From 782b0a934a22f5a43e32bd6b0786e273012227c1 Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Tue, 10 Jan 2023 15:28:07 -0500 Subject: [PATCH 2/9] tabledesc: lift GetIndexNameByIDMethod to catalog This commit refactors this method into a function. Informs #91924. Release note: None --- pkg/sql/backfill.go | 2 +- pkg/sql/catalog/descriptor.go | 6 ------ pkg/sql/catalog/table_elements.go | 27 +++++++++++++++++++++++++ pkg/sql/catalog/tabledesc/table_desc.go | 21 ------------------- pkg/sql/row/errors.go | 2 +- 5 files changed, 29 insertions(+), 29 deletions(-) diff --git a/pkg/sql/backfill.go b/pkg/sql/backfill.go index 5e098421e434..3595058ccdf0 100644 --- a/pkg/sql/backfill.go +++ b/pkg/sql/backfill.go @@ -1855,7 +1855,7 @@ func ValidateForwardIndexes( return nil } // Resolve the table index descriptor name. - indexName, err := tableDesc.GetIndexNameByID(idx.GetID()) + indexName, err := catalog.FindTargetIndexNameByID(tableDesc, idx.GetID()) if err != nil { log.Warningf(ctx, "unable to find index by ID for ValidateForwardIndexes: %d", diff --git a/pkg/sql/catalog/descriptor.go b/pkg/sql/catalog/descriptor.go index 5ceb28dfbf2c..2b2bc614d77c 100644 --- a/pkg/sql/catalog/descriptor.go +++ b/pkg/sql/catalog/descriptor.go @@ -748,12 +748,6 @@ type TableDescriptor interface { // enabled or disabled for this table. If ok is true, then the enabled value // is valid, otherwise this has not been set at the table level. ForecastStatsEnabled() (enabled bool, ok bool) - // GetIndexNameByID returns the name of an index based on an ID, taking into - // account any ongoing declarative schema changes. Declarative schema changes - // do not propagate the index name into the mutations until changes are fully - // validated and swap operations are complete (to avoid having two constraints - // with the same name). - GetIndexNameByID(indexID descpb.IndexID) (name string, err error) // IsRefreshViewRequired indicates if a REFRESH VIEW operation needs to be called // on a materialized view. IsRefreshViewRequired() bool diff --git a/pkg/sql/catalog/table_elements.go b/pkg/sql/catalog/table_elements.go index 8657622abb98..ba97ab024610 100644 --- a/pkg/sql/catalog/table_elements.go +++ b/pkg/sql/catalog/table_elements.go @@ -18,6 +18,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/catenumpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" "github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants" "github.com/cockroachdb/cockroach/pkg/sql/sem/semenumpb" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" @@ -1019,3 +1020,29 @@ func GetConstraintType(c Constraint) catconstants.ConstraintType { panic(errors.AssertionFailedf("unknown constraint type %T", c)) } } + +// FindTargetIndexNameByID returns the name of an index based on an ID, taking +// into account any ongoing declarative schema changes. Declarative schema +// changes do not propagate the index name into the mutations until changes are +// fully validated and swap operations are complete (to avoid having two +// constraints with the same name). +func FindTargetIndexNameByID(desc TableDescriptor, indexID descpb.IndexID) (string, error) { + // Check if there are any ongoing schema changes and prefer the name from + // them. + if scState := desc.GetDeclarativeSchemaChangerState(); scState != nil { + for _, target := range scState.Targets { + if target.IndexName != nil && + target.TargetStatus == scpb.Status_PUBLIC && + target.IndexName.TableID == desc.GetID() && + target.IndexName.IndexID == indexID { + return target.IndexName.Name, nil + } + } + } + // Otherwise, try fetching the name from the index descriptor. + index, err := desc.FindIndexWithID(indexID) + if err != nil { + return "", err + } + return index.GetName(), err +} diff --git a/pkg/sql/catalog/tabledesc/table_desc.go b/pkg/sql/catalog/tabledesc/table_desc.go index 05c913c7ec92..5145895823f9 100644 --- a/pkg/sql/catalog/tabledesc/table_desc.go +++ b/pkg/sql/catalog/tabledesc/table_desc.go @@ -671,27 +671,6 @@ func (desc *wrapper) AllMutations() []catalog.Mutation { return desc.getExistingOrNewMutationCache().all } -func (desc *wrapper) GetIndexNameByID(indexID descpb.IndexID) (string, error) { - // Check if there are any ongoing schema changes and prefer the name from - // them. - if scState := desc.GetDeclarativeSchemaChangerState(); scState != nil { - for _, target := range scState.Targets { - if target.IndexName != nil && - target.TargetStatus == scpb.Status_PUBLIC && - target.IndexName.TableID == desc.GetID() && - target.IndexName.IndexID == indexID { - return target.IndexName.Name, nil - } - } - } - // Otherwise, try fetching the name from the index descriptor. - index, err := desc.FindIndexWithID(indexID) - if err != nil { - return "", err - } - return index.GetName(), err -} - // IsRefreshViewRequired implements the TableDescriptor interface. func (desc *wrapper) IsRefreshViewRequired() bool { return desc.IsMaterializedView && desc.RefreshViewRequired diff --git a/pkg/sql/row/errors.go b/pkg/sql/row/errors.go index 56e3920f7a56..103cefdf986d 100644 --- a/pkg/sql/row/errors.go +++ b/pkg/sql/row/errors.go @@ -112,7 +112,7 @@ func NewUniquenessConstraintViolationError( "duplicate key value got decoding error") } // Resolve the table index descriptor name. - indexName, err := tableDesc.GetIndexNameByID(index.GetID()) + indexName, err := catalog.FindTargetIndexNameByID(tableDesc, index.GetID()) if err != nil { log.Warningf(ctx, "unable to find index by ID for NewUniquenessConstraintViolationError: %d", From 26b651658781e52d2cc4edfffcdde280fd402c97 Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Tue, 10 Jan 2023 16:08:51 -0500 Subject: [PATCH 3/9] catalog: remove CheckConstraintUsesColumn method This commit removes an unnecessary method. Informs #91924. Release note: None --- pkg/sql/alter_column_type.go | 8 ++------ pkg/sql/alter_table.go | 4 +--- pkg/sql/catalog/descriptor.go | 3 --- pkg/sql/catalog/tabledesc/structured.go | 11 ----------- pkg/sql/drop_index.go | 4 +--- pkg/sql/tests/hash_sharded_test.go | 6 +----- 6 files changed, 5 insertions(+), 31 deletions(-) diff --git a/pkg/sql/alter_column_type.go b/pkg/sql/alter_column_type.go index a7e66fc3dd56..50cee7b3c1c5 100644 --- a/pkg/sql/alter_column_type.go +++ b/pkg/sql/alter_column_type.go @@ -203,12 +203,8 @@ func alterColumnTypeGeneral( // Disallow ALTER COLUMN TYPE general for columns that have a check // constraint. - for i := range tableDesc.Checks { - uses, err := tableDesc.CheckConstraintUsesColumn(tableDesc.Checks[i], col.GetID()) - if err != nil { - return err - } - if uses { + for _, ck := range tableDesc.EnforcedCheckConstraints() { + if ck.CollectReferencedColumnIDs().Contains(col.GetID()) { return colWithConstraintNotSupportedErr } } diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index d680a9e5be44..e2fe3f349ae8 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -1704,9 +1704,7 @@ func dropColumnImpl( if check.Dropped() { continue } - if used, err := tableDesc.CheckConstraintUsesColumn(check.CheckDesc(), colToDrop.GetID()); err != nil { - return nil, err - } else if !used { + if !check.CollectReferencedColumnIDs().Contains(colToDrop.GetID()) { continue } if err := tableDesc.DropConstraint(check, nil /* removeFKBackRef */); err != nil { diff --git a/pkg/sql/catalog/descriptor.go b/pkg/sql/catalog/descriptor.go index 2b2bc614d77c..22443e20afec 100644 --- a/pkg/sql/catalog/descriptor.go +++ b/pkg/sql/catalog/descriptor.go @@ -563,9 +563,6 @@ type TableDescriptor interface { // GetNextConstraintID returns the next unused constraint ID for this table. // Constraint IDs are unique per table, but not unique globally. GetNextConstraintID() descpb.ConstraintID - // CheckConstraintUsesColumn returns whether the check constraint uses the - // specified column. - CheckConstraintUsesColumn(cc *descpb.TableDescriptor_CheckConstraint, colID descpb.ColumnID) (bool, error) // IsShardColumn returns true if col corresponds to a non-dropped hash sharded // index. This method assumes that col is currently a member of desc. IsShardColumn(col Column) bool diff --git a/pkg/sql/catalog/tabledesc/structured.go b/pkg/sql/catalog/tabledesc/structured.go index 075f2e992b71..5b657deeccec 100644 --- a/pkg/sql/catalog/tabledesc/structured.go +++ b/pkg/sql/catalog/tabledesc/structured.go @@ -13,7 +13,6 @@ package tabledesc import ( "context" "fmt" - "sort" "strconv" "strings" @@ -2115,16 +2114,6 @@ func (desc *wrapper) TableSpan(codec keys.SQLCodec) roachpb.Span { return roachpb.Span{Key: prefix, EndKey: prefix.PrefixEnd()} } -// CheckConstraintUsesColumn implements the TableDescriptor interface. -func (desc *wrapper) CheckConstraintUsesColumn( - cc *descpb.TableDescriptor_CheckConstraint, colID descpb.ColumnID, -) (bool, error) { - i := sort.Search(len(cc.ColumnIDs), func(i int) bool { - return cc.ColumnIDs[i] >= colID - }) - return i < len(cc.ColumnIDs) && cc.ColumnIDs[i] == colID, nil -} - // GetFamilyOfColumn returns the ColumnFamilyDescriptor for the // the family the column is part of. func (desc *wrapper) GetFamilyOfColumn( diff --git a/pkg/sql/drop_index.go b/pkg/sql/drop_index.go index 487bfe73105a..073ca414e7b7 100644 --- a/pkg/sql/drop_index.go +++ b/pkg/sql/drop_index.go @@ -247,9 +247,7 @@ func (n *dropIndexNode) dropShardColumnAndConstraint( ) error { validChecks := tableDesc.Checks[:0] for _, check := range tableDesc.CheckConstraints() { - if used, err := tableDesc.CheckConstraintUsesColumn(check.CheckDesc(), shardCol.GetID()); err != nil { - return err - } else if used { + if check.CollectReferencedColumnIDs().Contains(shardCol.GetID()) { if check.GetConstraintValidity() == descpb.ConstraintValidity_Validating { return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, "referencing constraint %q in the middle of being added, try again later", check.GetName()) diff --git a/pkg/sql/tests/hash_sharded_test.go b/pkg/sql/tests/hash_sharded_test.go index 8b59bc079ec9..5077572beb3d 100644 --- a/pkg/sql/tests/hash_sharded_test.go +++ b/pkg/sql/tests/hash_sharded_test.go @@ -61,11 +61,7 @@ func verifyTableDescriptorState( shardColID := getShardColumnID(t, tableDesc, shardedIndexName) foundCheckConstraint := false for _, check := range tableDesc.CheckConstraints() { - usesShard, err := tableDesc.CheckConstraintUsesColumn(check.CheckDesc(), shardColID) - if err != nil { - t.Fatal(err) - } - if usesShard && check.IsHashShardingConstraint() { + if check.CollectReferencedColumnIDs().Contains(shardColID) && check.IsHashShardingConstraint() { foundCheckConstraint = true break } From 6b20152f91e2013d87c2d9c3f43dc4efc9d9a84b Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Tue, 10 Jan 2023 16:20:53 -0500 Subject: [PATCH 4/9] catalog: replace ColumnNamesForIDs method with function This commit refacts this method as a function. Informs #91924. Release note: None --- pkg/sql/catalog/descriptor.go | 4 ---- pkg/sql/catalog/table_elements.go | 21 +++++++++++++++++++++ pkg/sql/catalog/tabledesc/structured.go | 13 ------------- pkg/sql/check.go | 8 ++++---- pkg/sql/create_table.go | 2 +- pkg/sql/pg_catalog.go | 2 +- pkg/sql/show_create_clauses.go | 6 +++--- 7 files changed, 30 insertions(+), 26 deletions(-) diff --git a/pkg/sql/catalog/descriptor.go b/pkg/sql/catalog/descriptor.go index 22443e20afec..2823b0677284 100644 --- a/pkg/sql/catalog/descriptor.go +++ b/pkg/sql/catalog/descriptor.go @@ -553,10 +553,6 @@ type TableDescriptor interface { // If no column is found then an error is also returned. FindColumnWithName(name tree.Name) (Column, error) - // NamesForColumnIDs returns the names for the given column ids, or an error - // if one or more column ids was missing. Note - this allocates! It's not for - // hot path code. - NamesForColumnIDs(ids descpb.ColumnIDs) ([]string, error) // GetNextColumnID returns the next unused column ID for this table. Column // IDs are unique per table, but not unique globally. GetNextColumnID() descpb.ColumnID diff --git a/pkg/sql/catalog/table_elements.go b/pkg/sql/catalog/table_elements.go index ba97ab024610..14aa23c6ee91 100644 --- a/pkg/sql/catalog/table_elements.go +++ b/pkg/sql/catalog/table_elements.go @@ -1046,3 +1046,24 @@ func FindTargetIndexNameByID(desc TableDescriptor, indexID descpb.IndexID) (stri } return index.GetName(), err } + +// ColumnNamesForIDs returns the names for the given column IDs, or an error +// if one or more column ids was missing. Note - this allocates! It's not for +// hot path code. +func ColumnNamesForIDs(tbl TableDescriptor, ids descpb.ColumnIDs) ([]string, error) { + names := make([]string, len(ids)) + columns := tbl.AllColumns() + for i, id := range ids { + for _, c := range columns { + if c.GetID() == id { + names[i] = c.GetName() + break + } + } + if names[i] == "" { + return nil, errors.AssertionFailedf("no column with ID %d found in table %q (%q)", + id, tbl.GetName(), tbl.GetID()) + } + } + return names, nil +} diff --git a/pkg/sql/catalog/tabledesc/structured.go b/pkg/sql/catalog/tabledesc/structured.go index 5b657deeccec..a20b7365e606 100644 --- a/pkg/sql/catalog/tabledesc/structured.go +++ b/pkg/sql/catalog/tabledesc/structured.go @@ -1167,19 +1167,6 @@ func (desc *wrapper) FindFamilyByID(id descpb.FamilyID) (*descpb.ColumnFamilyDes return nil, fmt.Errorf("family-id \"%d\" does not exist", id) } -// NamesForColumnIDs implements the TableDescriptor interface. -func (desc *wrapper) NamesForColumnIDs(ids descpb.ColumnIDs) ([]string, error) { - names := make([]string, len(ids)) - for i, id := range ids { - col, err := desc.FindColumnWithID(id) - if err != nil { - return nil, err - } - names[i] = col.GetName() - } - return names, nil -} - // DropConstraint drops a constraint, either by removing it from the table // descriptor or by queuing a mutation for a schema change. func (desc *Mutable) DropConstraint( diff --git a/pkg/sql/check.go b/pkg/sql/check.go index 09c922b44965..89c31713304c 100644 --- a/pkg/sql/check.go +++ b/pkg/sql/check.go @@ -183,7 +183,7 @@ func nonMatchingRowQuery( indexIDForValidation descpb.IndexID, limitResults bool, ) (sql string, originColNames []string, _ error) { - originColNames, err := srcTbl.NamesForColumnIDs(fk.OriginColumnIDs) + originColNames, err := catalog.ColumnNamesForIDs(srcTbl, fk.OriginColumnIDs) if err != nil { return "", nil, err } @@ -213,7 +213,7 @@ func nonMatchingRowQuery( qualifiedSrcCols[i] = fmt.Sprintf("s.%s", srcCols[i]) } - referencedColNames, err := targetTbl.NamesForColumnIDs(fk.ReferencedColumnIDs) + referencedColNames, err := catalog.ColumnNamesForIDs(targetTbl, fk.ReferencedColumnIDs) if err != nil { return "", nil, err } @@ -288,7 +288,7 @@ func validateForeignKey( ) error { nCols := len(fk.OriginColumnIDs) - referencedColumnNames, err := targetTable.NamesForColumnIDs(fk.ReferencedColumnIDs) + referencedColumnNames, err := catalog.ColumnNamesForIDs(targetTable, fk.ReferencedColumnIDs) if err != nil { return err } @@ -375,7 +375,7 @@ func duplicateRowQuery( indexIDForValidation descpb.IndexID, limitResults bool, ) (sql string, colNames []string, _ error) { - colNames, err := srcTbl.NamesForColumnIDs(columnIDs) + colNames, err := catalog.ColumnNamesForIDs(srcTbl, columnIDs) if err != nil { return "", nil, err } diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go index 60b9028f80bb..73ad24b46273 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -2557,7 +2557,7 @@ func replaceLikeTableOpts(n *tree.CreateTable, params runParams) (tree.TableDefs }, WithoutIndex: true, } - colNames, err := td.NamesForColumnIDs(c.ColumnIDs) + colNames, err := catalog.ColumnNamesForIDs(td, c.ColumnIDs) if err != nil { return nil, err } diff --git a/pkg/sql/pg_catalog.go b/pkg/sql/pg_catalog.go index 103d0f031a6e..7f588ec3f028 100644 --- a/pkg/sql/pg_catalog.go +++ b/pkg/sql/pg_catalog.go @@ -985,7 +985,7 @@ func populateTableConstraints( db.GetID(), sc.GetID(), table.GetID(), uwoi, ) f.WriteString("UNIQUE WITHOUT INDEX (") - colNames, err := table.NamesForColumnIDs(uwoi.UniqueWithoutIndexDesc().ColumnIDs) + colNames, err := catalog.ColumnNamesForIDs(table, uwoi.UniqueWithoutIndexDesc().ColumnIDs) if err != nil { return err } diff --git a/pkg/sql/show_create_clauses.go b/pkg/sql/show_create_clauses.go index 3fc2332b8c69..6ac6b9097c5f 100644 --- a/pkg/sql/show_create_clauses.go +++ b/pkg/sql/show_create_clauses.go @@ -449,11 +449,11 @@ func showForeignKeyConstraint( return err } fkTableName.ExplicitSchema = !searchPath.Contains(fkTableName.SchemaName.String(), false /* includeImplicit */) - refNames, err = fkTable.NamesForColumnIDs(fk.ReferencedColumnIDs) + refNames, err = catalog.ColumnNamesForIDs(fkTable, fk.ReferencedColumnIDs) if err != nil { return err } - originNames, err = originTable.NamesForColumnIDs(fk.OriginColumnIDs) + originNames, err = catalog.ColumnNamesForIDs(originTable, fk.OriginColumnIDs) if err != nil { return err } @@ -725,7 +725,7 @@ func showConstraintClause( f.WriteString(" ") } f.WriteString("UNIQUE WITHOUT INDEX (") - colNames, err := desc.NamesForColumnIDs(c.CollectKeyColumnIDs().Ordered()) + colNames, err := catalog.ColumnNamesForIDs(desc, c.CollectKeyColumnIDs().Ordered()) if err != nil { return err } From 0682851cda2cd29c7e9e033ec7e1c4d804ef5d06 Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Wed, 11 Jan 2023 10:09:27 -0500 Subject: [PATCH 5/9] catalog: replace FindIndexWith* methods with functions This commit refactors these methods as functions. Informs #91924. Release note: None --- pkg/ccl/backupccl/restore_job.go | 2 +- pkg/ccl/partitionccl/drop_test.go | 5 +- pkg/ccl/partitionccl/partition_test.go | 5 +- pkg/ccl/partitionccl/zone_test.go | 4 +- pkg/ccl/testccl/sqlccl/BUILD.bazel | 1 + pkg/ccl/testccl/sqlccl/tenant_gc_test.go | 3 +- pkg/server/BUILD.bazel | 1 + pkg/server/status.go | 5 +- pkg/sql/alter_index_visible.go | 4 +- pkg/sql/alter_primary_key.go | 5 +- pkg/sql/alter_table.go | 12 ++-- pkg/sql/catalog/descriptor.go | 17 ------ pkg/sql/catalog/funcdesc/func_desc.go | 3 +- pkg/sql/catalog/resolver/resolver.go | 56 +++++++++++-------- pkg/sql/catalog/table_elements.go | 43 +++++++++++++- pkg/sql/catalog/tabledesc/index_test.go | 4 +- pkg/sql/catalog/tabledesc/structured.go | 6 +- pkg/sql/catalog/tabledesc/structured_test.go | 2 +- pkg/sql/catalog/tabledesc/table_desc.go | 49 ---------------- .../catalog/tabledesc/table_desc_builder.go | 4 +- pkg/sql/catalog/tabledesc/validate.go | 4 +- pkg/sql/crdb_internal.go | 2 +- pkg/sql/create_index.go | 6 +- pkg/sql/create_table.go | 4 +- pkg/sql/delete_preserving_index_test.go | 10 ++-- pkg/sql/descriptor_mutation_test.go | 3 +- pkg/sql/drop_index.go | 4 +- pkg/sql/drop_test.go | 14 ++--- pkg/sql/evalcatalog/encode_table_index_key.go | 2 +- .../evalcatalog/geo_inverted_index_entries.go | 3 +- pkg/sql/indexbackfiller_test.go | 2 +- pkg/sql/rename_index.go | 4 +- pkg/sql/resolver.go | 2 +- pkg/sql/row/errors.go | 2 +- pkg/sql/rowenc/index_encoding_test.go | 2 +- pkg/sql/rowenc/index_fetch_test.go | 3 +- pkg/sql/rowexec/utils_test.go | 2 +- pkg/sql/schema_changer_test.go | 2 +- .../scdeps/sctestdeps/test_deps.go | 16 +++++- .../scexec/exec_backfill_test.go | 2 +- .../schemachanger/scexec/exec_validation.go | 2 +- .../scexec/scmutationexec/eventlog.go | 3 +- .../scexec/scmutationexec/index.go | 12 ++-- pkg/sql/show_create_clauses.go | 2 +- pkg/sql/span/BUILD.bazel | 1 + pkg/sql/span/span_splitter_test.go | 3 +- pkg/sql/tests/hash_sharded_test.go | 6 +- pkg/sql/unsplit_range_test.go | 2 +- pkg/sql/zone_config.go | 2 +- .../sampled_stmt_diagnostics_requests.go | 2 +- pkg/upgrade/upgrades/schema_changes.go | 13 ++--- 51 files changed, 186 insertions(+), 182 deletions(-) diff --git a/pkg/ccl/backupccl/restore_job.go b/pkg/ccl/backupccl/restore_job.go index b59fbb89e543..5e21bbd1befe 100644 --- a/pkg/ccl/backupccl/restore_job.go +++ b/pkg/ccl/backupccl/restore_job.go @@ -2116,7 +2116,7 @@ func (r *restoreResumer) publishDescriptors( badIndexes := devalidateIndexes[mutTable.ID] for _, badIdx := range badIndexes { - found, err := mutTable.FindIndexWithID(badIdx) + found, err := catalog.MustFindIndexByID(mutTable, badIdx) if err != nil { return err } diff --git a/pkg/ccl/partitionccl/drop_test.go b/pkg/ccl/partitionccl/drop_test.go index 9cd4af2afe8b..740e06e8c794 100644 --- a/pkg/ccl/partitionccl/drop_test.go +++ b/pkg/ccl/partitionccl/drop_test.go @@ -18,6 +18,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/jobs" "github.com/cockroachdb/cockroach/pkg/jobs/jobspb" "github.com/cockroachdb/cockroach/pkg/sql" + "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" "github.com/cockroachdb/cockroach/pkg/sql/gcjob" "github.com/cockroachdb/cockroach/pkg/sql/tests" @@ -74,7 +75,7 @@ func TestDropIndexWithZoneConfigCCL(t *testing.T) { )`) codec := tenantOrSystemCodec(s) tableDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, codec, "t", "kv") - index, err := tableDesc.FindIndexWithName("i") + index, err := catalog.MustFindIndexByName(tableDesc, "i") if err != nil { t.Fatal(err) } @@ -121,7 +122,7 @@ func TestDropIndexWithZoneConfigCCL(t *testing.T) { } } tableDesc = desctestutils.TestingGetPublicTableDescriptor(kvDB, codec, "t", "kv") - if _, err := tableDesc.FindIndexWithName("i"); err == nil { + if catalog.FindIndexByName(tableDesc, "i") != nil { t.Fatalf("table descriptor still contains index after index is dropped") } close(asyncNotification) diff --git a/pkg/ccl/partitionccl/partition_test.go b/pkg/ccl/partitionccl/partition_test.go index e309e0fbaed3..5d61832cd7b7 100644 --- a/pkg/ccl/partitionccl/partition_test.go +++ b/pkg/ccl/partitionccl/partition_test.go @@ -31,6 +31,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/server" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql" + "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/bootstrap" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" @@ -204,7 +205,7 @@ func (pt *partitioningTest) parse() error { if !strings.HasPrefix(indexName, "@") { panic(errors.Errorf("unsupported config: %s", c)) } - idx, err := pt.parsed.tableDesc.FindIndexWithName(indexName[1:]) + idx, err := catalog.MustFindIndexByName(pt.parsed.tableDesc, indexName[1:]) if err != nil { return errors.Wrapf(err, "could not find index %s", indexName) } @@ -1280,7 +1281,7 @@ func TestRepartitioning(t *testing.T) { } sqlDB.Exec(t, fmt.Sprintf("ALTER TABLE %s RENAME TO %s", test.old.parsed.tableName, test.new.parsed.tableName)) - testIndex, err := test.new.parsed.tableDesc.FindIndexWithName(test.index) + testIndex, err := catalog.MustFindIndexByName(test.new.parsed.tableDesc, test.index) if err != nil { t.Fatalf("%+v", err) } diff --git a/pkg/ccl/partitionccl/zone_test.go b/pkg/ccl/partitionccl/zone_test.go index 38aedfced5a3..732290675e30 100644 --- a/pkg/ccl/partitionccl/zone_test.go +++ b/pkg/ccl/partitionccl/zone_test.go @@ -312,7 +312,7 @@ func TestGenerateSubzoneSpans(t *testing.T) { var actual []string for _, span := range spans { subzone := test.parsed.subzones[span.SubzoneIndex] - idx, err := test.parsed.tableDesc.FindIndexWithID(descpb.IndexID(subzone.IndexID)) + idx, err := catalog.MustFindIndexByID(test.parsed.tableDesc, descpb.IndexID(subzone.IndexID)) if err != nil { t.Fatalf("could not find index with ID %d: %+v", subzone.IndexID, err) } @@ -432,7 +432,7 @@ PARTITION p1 VALUES IN (DEFAULT));`) // Find the temporary index corresponding to the new index. tbl := desctestutils.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "t", "public", "test") - newIndex, err := tbl.FindIndexWithName("idx") + newIndex, err := catalog.MustFindIndexByName(tbl, "idx") if err != nil { t.Fatal(err) } diff --git a/pkg/ccl/testccl/sqlccl/BUILD.bazel b/pkg/ccl/testccl/sqlccl/BUILD.bazel index c1b2d2096c5b..b4ba0c47aa5a 100644 --- a/pkg/ccl/testccl/sqlccl/BUILD.bazel +++ b/pkg/ccl/testccl/sqlccl/BUILD.bazel @@ -30,6 +30,7 @@ go_test( "//pkg/settings/cluster", "//pkg/spanconfig", "//pkg/sql", + "//pkg/sql/catalog", "//pkg/sql/catalog/descpb", "//pkg/sql/catalog/desctestutils", "//pkg/sql/gcjob", diff --git a/pkg/ccl/testccl/sqlccl/tenant_gc_test.go b/pkg/ccl/testccl/sqlccl/tenant_gc_test.go index 4f5e0c69dae8..210b32896bff 100644 --- a/pkg/ccl/testccl/sqlccl/tenant_gc_test.go +++ b/pkg/ccl/testccl/sqlccl/tenant_gc_test.go @@ -25,6 +25,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/spanconfig" "github.com/cockroachdb/cockroach/pkg/sql" + "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" "github.com/cockroachdb/cockroach/pkg/sql/gcjob" @@ -325,7 +326,7 @@ func TestGCTableOrIndexWaitsForProtectedTimestamps(t *testing.T) { tableDesc := desctestutils.TestingGetTableDescriptor(execCfg.DB, execCfg.Codec, "db", "public", "t") tableID := tableDesc.GetID() - idx, err := tableDesc.FindIndexWithName("t_idx") + idx, err := catalog.MustFindIndexByName(tableDesc, "t_idx") require.NoError(t, err) indexID := idx.GetID() diff --git a/pkg/server/BUILD.bazel b/pkg/server/BUILD.bazel index 2e783110064c..302079112deb 100644 --- a/pkg/server/BUILD.bazel +++ b/pkg/server/BUILD.bazel @@ -164,6 +164,7 @@ go_library( "//pkg/spanconfig/spanconfigsqltranslator", "//pkg/spanconfig/spanconfigsqlwatcher", "//pkg/sql", + "//pkg/sql/catalog", "//pkg/sql/catalog/bootstrap", "//pkg/sql/catalog/catalogkeys", "//pkg/sql/catalog/catsessiondata", diff --git a/pkg/server/status.go b/pkg/server/status.go index ded6e118d53b..8b0ed83b6204 100644 --- a/pkg/server/status.go +++ b/pkg/server/status.go @@ -52,6 +52,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/spanconfig" "github.com/cockroachdb/cockroach/pkg/sql" + "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/clusterunique" @@ -2538,8 +2539,8 @@ func (s *systemStatusServer) HotRangesV2( if _, _, idxID, err := s.sqlServer.execCfg.Codec.DecodeIndexPrefix(r.Desc.StartKey.AsRawKey()); err != nil { log.Warningf(ctx, "cannot decode index prefix for range descriptor: %s: %v", r.Desc, err) } else { - if index, err := desc.FindIndexWithID(descpb.IndexID(idxID)); err != nil { - log.Warningf(ctx, "cannot get index name for range descriptor: %s: %v", r.Desc, err) + if index := catalog.FindIndexByID(desc, descpb.IndexID(idxID)); index == nil { + log.Warningf(ctx, "cannot get index name for range descriptor: %s: index with ID %d not found", r.Desc, idxID) } else { indexName = index.GetName() } diff --git a/pkg/sql/alter_index_visible.go b/pkg/sql/alter_index_visible.go index b514e0c0ab07..c4dd11da80dc 100644 --- a/pkg/sql/alter_index_visible.go +++ b/pkg/sql/alter_index_visible.go @@ -53,9 +53,9 @@ func (p *planner) AlterIndexVisible( return newZeroNode(nil /* columns */), nil } - // Check if the index actually exists. FindIndexWithName returns the first + // Check if the index actually exists. MustFindIndexByName returns the first // catalog.Index in tableDesc.AllIndexes(). - idx, err := tableDesc.FindIndexWithName(string(n.Index.Index)) + idx, err := catalog.MustFindIndexByName(tableDesc, string(n.Index.Index)) if err != nil { if n.IfExists { // Nothing needed if no index exists and IfExists is true. diff --git a/pkg/sql/alter_primary_key.go b/pkg/sql/alter_primary_key.go index de592169bc48..d77f9e66d57b 100644 --- a/pkg/sql/alter_primary_key.go +++ b/pkg/sql/alter_primary_key.go @@ -163,8 +163,7 @@ func (p *planner) AlterPrimaryKey( } nameExists := func(name string) bool { - _, err := tableDesc.FindIndexWithName(name) - return err == nil + return catalog.FindIndexByName(tableDesc, name) != nil } // Make a new index that is suitable to be a primary index. @@ -362,7 +361,7 @@ func (p *planner) AlterPrimaryKey( // as most of this code relies upon for correctness. This code will // all be replaced by code in the declarative schema changer before // too long where we'll model this all correctly. - newTempPrimaryIndex, err := tableDesc.FindIndexWithID(newPrimaryIndexDesc.ID + 1) + newTempPrimaryIndex, err := catalog.MustFindIndexByID(tableDesc, newPrimaryIndexDesc.ID+1) if err != nil { return errors.NewAssertionErrorWithWrappedErrf(err, "failed to find newly created temporary index for backfill") diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index e2fe3f349ae8..74c7bc2c8621 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -311,12 +311,10 @@ func (n *alterTableNode) startExec(params runParams) error { if err != nil { return err } - foundIndex, err := n.tableDesc.FindIndexWithName(string(d.Name)) - if err == nil { - if foundIndex.Dropped() { - return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, - "index %q being dropped, try again later", d.Name) - } + foundIndex := catalog.FindIndexByName(n.tableDesc, string(d.Name)) + if foundIndex != nil && foundIndex.Dropped() { + return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, + "index %q being dropped, try again later", d.Name) } if err := n.tableDesc.AddIndexMutationMaybeWithTempIndex( &idx, descpb.DescriptorMutation_ADD, @@ -1431,7 +1429,7 @@ func validateConstraintNameIsNotUsed( if name == "" { return false, nil } - idx, _ := tableDesc.FindIndexWithName(string(name)) + idx := catalog.FindIndexByName(tableDesc, string(name)) // If an index is found and its disabled, then we know it will be dropped // later on. if idx == nil { diff --git a/pkg/sql/catalog/descriptor.go b/pkg/sql/catalog/descriptor.go index 2823b0677284..2be6eefb0981 100644 --- a/pkg/sql/catalog/descriptor.go +++ b/pkg/sql/catalog/descriptor.go @@ -429,23 +429,6 @@ type TableDescriptor interface { // state. DeleteOnlyNonPrimaryIndexes() []Index - // FindIndexWithID returns the first catalog.Index that matches the id - // in the set of all indexes, or an error if none was found. The order of - // traversal is the canonical order, see Index.Ordinal(). - FindIndexWithID(id descpb.IndexID) (Index, error) - - // FindIndexWithName returns the first catalog.Index that matches the name in - // the set of all indexes, excluding the primary index of non-physical - // tables, or an error if none was found. The order of traversal is the - // canonical order, see Index.Ordinal(). - FindIndexWithName(name string) (Index, error) - - // FindNonDropIndexWithName returns the first catalog.Index that matches the name in - // the set of all non-drp[ indexes, excluding the primary index of non-physical - // tables, or an error if none was found. The order of traversal is the - // canonical order, see catalog.Index.Ordinal(). - FindNonDropIndexWithName(name string) (Index, error) - // GetNextIndexID returns the next unused index ID for the table. Index IDs // are unique within a table, but not globally. GetNextIndexID() descpb.IndexID diff --git a/pkg/sql/catalog/funcdesc/func_desc.go b/pkg/sql/catalog/funcdesc/func_desc.go index 75ef03f3c824..3834293084ba 100644 --- a/pkg/sql/catalog/funcdesc/func_desc.go +++ b/pkg/sql/catalog/funcdesc/func_desc.go @@ -321,8 +321,7 @@ func (desc *immutable) validateInboundTableRef( } for _, idxID := range by.IndexIDs { - _, err := backRefTbl.FindIndexWithID(idxID) - if err != nil { + if catalog.FindIndexByID(backRefTbl, idxID) == nil { return errors.AssertionFailedf("depended-on-by relation %q (%d) does not have an index with ID %d", backRefTbl.GetName(), by.ID, idxID) } diff --git a/pkg/sql/catalog/resolver/resolver.go b/pkg/sql/catalog/resolver/resolver.go index e1aefd595488..ce6a5d909d3d 100644 --- a/pkg/sql/catalog/resolver/resolver.go +++ b/pkg/sql/catalog/resolver/resolver.go @@ -531,9 +531,14 @@ func ResolveIndex( return true, resolvedPrefix, candidateTbl, candidateTbl.GetPrimaryIndex(), nil } - idx, err := candidateTbl.FindNonDropIndexWithName(string(tableIndexName.Index)) - // err == nil indicates that the index is found. - if err == nil && (flag.IncludeNonActiveIndex || idx.Public()) { + idx := catalog.FindIndex( + candidateTbl, + catalog.IndexOpts{AddMutations: true}, + func(idx catalog.Index) bool { + return idx.GetName() == string(tableIndexName.Index) + }, + ) + if idx != nil && (flag.IncludeNonActiveIndex || idx.Public()) { return true, resolvedPrefix, candidateTbl, idx, nil } @@ -738,26 +743,33 @@ func findTableContainingIndex( continue } - candidateIdx, err := candidateTbl.FindNonDropIndexWithName(string(indexName)) - if err == nil { - if !includeNonActiveIndex && !candidateIdx.Public() { - continue - } - if found { - prefix := resolvedPrefix.NamePrefix() - tn1 := tree.MakeTableNameWithSchema(prefix.CatalogName, prefix.SchemaName, tree.Name(candidateTbl.GetName())) - tn2 := tree.MakeTableNameWithSchema(prefix.CatalogName, prefix.SchemaName, tree.Name(tblDesc.GetName())) - return false, nil, nil, pgerror.Newf( - pgcode.AmbiguousParameter, "index name %q is ambiguous (found in %s and %s)", - indexName, - tn1.String(), - tn2.String(), - ) - } - found = true - tblDesc = candidateTbl - idxDesc = candidateIdx + candidateIdx := catalog.FindIndex( + candidateTbl, + catalog.IndexOpts{AddMutations: true}, + func(idx catalog.Index) bool { + return idx.GetName() == string(indexName) + }, + ) + if candidateIdx == nil { + continue + } + if !includeNonActiveIndex && !candidateIdx.Public() { + continue + } + if found { + prefix := resolvedPrefix.NamePrefix() + tn1 := tree.MakeTableNameWithSchema(prefix.CatalogName, prefix.SchemaName, tree.Name(candidateTbl.GetName())) + tn2 := tree.MakeTableNameWithSchema(prefix.CatalogName, prefix.SchemaName, tree.Name(tblDesc.GetName())) + return false, nil, nil, pgerror.Newf( + pgcode.AmbiguousParameter, "index name %q is ambiguous (found in %s and %s)", + indexName, + tn1.String(), + tn2.String(), + ) } + found = true + tblDesc = candidateTbl + idxDesc = candidateIdx } return found, tblDesc, idxDesc, nil diff --git a/pkg/sql/catalog/table_elements.go b/pkg/sql/catalog/table_elements.go index 14aa23c6ee91..66446dbe84d6 100644 --- a/pkg/sql/catalog/table_elements.go +++ b/pkg/sql/catalog/table_elements.go @@ -1040,7 +1040,7 @@ func FindTargetIndexNameByID(desc TableDescriptor, indexID descpb.IndexID) (stri } } // Otherwise, try fetching the name from the index descriptor. - index, err := desc.FindIndexWithID(indexID) + index, err := MustFindIndexByID(desc, indexID) if err != nil { return "", err } @@ -1067,3 +1067,44 @@ func ColumnNamesForIDs(tbl TableDescriptor, ids descpb.ColumnIDs) ([]string, err } return names, nil } + +// FindIndexByID returns the first Index that matches the ID +// in the set of all indexes, or nil if none was found. +// The order of traversal is the canonical order, see Index.Ordinal(). +func FindIndexByID(tbl TableDescriptor, id descpb.IndexID) Index { + return FindIndex(tbl, IndexOpts{ + NonPhysicalPrimaryIndex: true, + DropMutations: true, + AddMutations: true, + }, func(idx Index) bool { + return idx.GetID() == id + }) +} + +// MustFindIndexByID is like FindIndexByID but returns an error when no index +// was found. +func MustFindIndexByID(tbl TableDescriptor, id descpb.IndexID) (Index, error) { + if idx := FindIndexByID(tbl, id); idx != nil { + return idx, nil + } + return nil, errors.Errorf("index-id \"%d\" does not exist", id) +} + +// FindIndexByName is like FindIndexByID but with names instead of IDs. +func FindIndexByName(tbl TableDescriptor, name string) Index { + return FindIndex(tbl, IndexOpts{ + NonPhysicalPrimaryIndex: true, + DropMutations: true, + AddMutations: true, + }, func(idx Index) bool { + return idx.GetName() == name + }) +} + +// MustFindIndexByName is like MustFindIndexByID but with names instead of IDs. +func MustFindIndexByName(tbl TableDescriptor, name string) (Index, error) { + if idx := FindIndexByName(tbl, name); idx != nil { + return idx, nil + } + return nil, errors.Errorf("index %q does not exist", name) +} diff --git a/pkg/sql/catalog/tabledesc/index_test.go b/pkg/sql/catalog/tabledesc/index_test.go index 596f26e55356..5bfe69d0949f 100644 --- a/pkg/sql/catalog/tabledesc/index_test.go +++ b/pkg/sql/catalog/tabledesc/index_test.go @@ -98,7 +98,7 @@ func TestIndexInterface(t *testing.T) { // are in the correct order. indexes := make([]catalog.Index, len(indexNames)) for i, name := range indexNames { - idx, err := tableI.FindIndexWithName(name) + idx, err := catalog.MustFindIndexByName(tableI, name) require.NoError(t, err) require.Equal(t, name, idx.GetName()) require.Equal(t, i, idx.Ordinal()) @@ -235,7 +235,7 @@ func TestIndexInterface(t *testing.T) { // Check that finding indexes by ID is correct. for _, idx := range indexes { - found, err := tableI.FindIndexWithID(idx.GetID()) + found, err := catalog.MustFindIndexByID(tableI, idx.GetID()) require.NoError(t, err) require.Equalf(t, idx.GetID(), found.GetID(), "mismatched IDs for index '%s'", idx.GetName()) diff --git a/pkg/sql/catalog/tabledesc/structured.go b/pkg/sql/catalog/tabledesc/structured.go index a20b7365e606..7201a8d66547 100644 --- a/pkg/sql/catalog/tabledesc/structured.go +++ b/pkg/sql/catalog/tabledesc/structured.go @@ -186,7 +186,7 @@ func BuildIndexName(tableDesc *Mutable, idx *descpb.IndexDescriptor) (string, er baseName := strings.Join(segments, "_") name := baseName for i := 1; ; i++ { - foundIndex, _ := tableDesc.FindIndexWithName(name) + foundIndex := catalog.FindIndexByName(tableDesc, name) if foundIndex == nil { break } @@ -1472,7 +1472,7 @@ func (desc *Mutable) MakeMutationComplete(m descpb.DescriptorMutation) error { // Promote the new primary index into the primary index position on the descriptor, // and remove it from the secondary indexes list. - newIndex, err := desc.FindIndexWithID(args.NewPrimaryIndexId) + newIndex, err := catalog.MustFindIndexByID(desc, args.NewPrimaryIndexId) if err != nil { return err } @@ -1509,7 +1509,7 @@ func (desc *Mutable) MakeMutationComplete(m descpb.DescriptorMutation) error { newID := args.NewIndexes[j] // All our new indexes have been inserted into the table descriptor by now, since the primary key swap // is the last mutation processed in a group of mutations under the same mutation ID. - newIndex, err := desc.FindIndexWithID(newID) + newIndex, err := catalog.MustFindIndexByID(desc, newID) if err != nil { return err } diff --git a/pkg/sql/catalog/tabledesc/structured_test.go b/pkg/sql/catalog/tabledesc/structured_test.go index 56e73ad3344d..aec8b55e6372 100644 --- a/pkg/sql/catalog/tabledesc/structured_test.go +++ b/pkg/sql/catalog/tabledesc/structured_test.go @@ -743,7 +743,7 @@ func TestKeysPerRow(t *testing.T) { desc := desctestutils.TestingGetPublicTableDescriptor(db, keys.SystemSQLCodec, "d", tableName) require.NotNil(t, desc) - idx, err := desc.FindIndexWithID(test.indexID) + idx, err := catalog.MustFindIndexByID(desc, test.indexID) require.NoError(t, err) keys := desc.IndexKeysPerRow(idx) if test.expected != keys { diff --git a/pkg/sql/catalog/tabledesc/table_desc.go b/pkg/sql/catalog/tabledesc/table_desc.go index 5145895823f9..d9005b01304d 100644 --- a/pkg/sql/catalog/tabledesc/table_desc.go +++ b/pkg/sql/catalog/tabledesc/table_desc.go @@ -365,55 +365,6 @@ func (desc *wrapper) DeleteOnlyNonPrimaryIndexes() []catalog.Index { return desc.getExistingOrNewIndexCache().deleteOnlyNonPrimary } -// FindIndexWithID returns the first catalog.Index that matches the id -// in the set of all indexes, or an error if none was found. The order of -// traversal is the canonical order, see catalog.Index.Ordinal(). -func (desc *wrapper) FindIndexWithID(id descpb.IndexID) (catalog.Index, error) { - if idx := catalog.FindIndex(desc, catalog.IndexOpts{ - NonPhysicalPrimaryIndex: true, - DropMutations: true, - AddMutations: true, - }, func(idx catalog.Index) bool { - return idx.GetID() == id - }); idx != nil { - return idx, nil - } - return nil, errors.Errorf("index-id \"%d\" does not exist", id) -} - -// FindIndexWithName returns the first catalog.Index that matches the name in -// the set of all indexes, excluding the primary index of non-physical -// tables, or an error if none was found. The order of traversal is the -// canonical order, see catalog.Index.Ordinal(). -func (desc *wrapper) FindIndexWithName(name string) (catalog.Index, error) { - if idx := catalog.FindIndex(desc, catalog.IndexOpts{ - NonPhysicalPrimaryIndex: false, - DropMutations: true, - AddMutations: true, - }, func(idx catalog.Index) bool { - return idx.GetName() == name - }); idx != nil { - return idx, nil - } - return nil, errors.Errorf("index %q does not exist", name) -} - -// FindNonDropIndexWithName returns the first catalog.Index that matches the name in -// the set of all non-drop indexes, excluding the primary index of non-physical -// tables, or an error if none was found. The order of traversal is the -// canonical order, see catalog.Index.Ordinal(). -func (desc *wrapper) FindNonDropIndexWithName(name string) (catalog.Index, error) { - if idx := catalog.FindIndex( - desc, - catalog.IndexOpts{AddMutations: true}, - func(idx catalog.Index) bool { - return idx.GetName() == name - }); idx != nil { - return idx, nil - } - return nil, errors.Errorf("index %q does not exist", name) -} - // getExistingOrNewColumnCache should be the only place where the columnCache // field in wrapper is ever read. func (desc *wrapper) getExistingOrNewColumnCache() *columnCache { diff --git a/pkg/sql/catalog/tabledesc/table_desc_builder.go b/pkg/sql/catalog/tabledesc/table_desc_builder.go index 4d667b78ae0a..ad3f594ddf91 100644 --- a/pkg/sql/catalog/tabledesc/table_desc_builder.go +++ b/pkg/sql/catalog/tabledesc/table_desc_builder.go @@ -422,7 +422,7 @@ func maybeUpgradeForeignKeyRepOnIndex( return false, err } if tbl, ok := otherUnupgradedTables[ref.Table]; ok { - referencedIndex, err := tbl.FindIndexWithID(ref.Index) + referencedIndex, err := catalog.MustFindIndexByID(tbl, ref.Index) if err != nil { return false, err } @@ -452,7 +452,7 @@ func maybeUpgradeForeignKeyRepOnIndex( return false, err } if otherTable, ok := otherUnupgradedTables[ref.Table]; ok { - originIndexI, err := otherTable.FindIndexWithID(ref.Index) + originIndexI, err := catalog.MustFindIndexByID(otherTable, ref.Index) if err != nil { return false, err } diff --git a/pkg/sql/catalog/tabledesc/validate.go b/pkg/sql/catalog/tabledesc/validate.go index 91d8ab4c1ec6..d7f2439b3cf2 100644 --- a/pkg/sql/catalog/tabledesc/validate.go +++ b/pkg/sql/catalog/tabledesc/validate.go @@ -609,7 +609,7 @@ func (desc *wrapper) ValidateSelf(vea catalog.ValidationErrorAccumulator) { // validation. for _, ref := range desc.DependedOnBy { if ref.IndexID != 0 { - if idx, _ := desc.FindIndexWithID(ref.IndexID); idx == nil { + if catalog.FindIndexByID(desc, ref.IndexID) == nil { vea.Report(errors.AssertionFailedf( "index ID %d found in depended-on-by references, no such index in this relation", ref.IndexID)) @@ -1357,7 +1357,7 @@ func (desc *wrapper) validateTableIndexes(columnsByID map[descpb.ColumnID]catalo for _, mut := range desc.Mutations { if mut.GetPrimaryKeySwap() != nil { newPKIdxID := mut.GetPrimaryKeySwap().NewPrimaryIndexId - newPK, err := desc.FindIndexWithID(newPKIdxID) + newPK, err := catalog.MustFindIndexByID(desc, newPKIdxID) if err != nil { return err } diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 4df2bb26762c..c3eef782ddce 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -7266,7 +7266,7 @@ func convertContentionEventsToJSON( return nil, err } - idxDesc, err := tableDesc.FindIndexWithID(descpb.IndexID(indexID)) + idxDesc, err := catalog.MustFindIndexByID(tableDesc, descpb.IndexID(indexID)) if err != nil { return nil, err } diff --git a/pkg/sql/create_index.go b/pkg/sql/create_index.go index 0889257dae5c..c2d1239b6ce9 100644 --- a/pkg/sql/create_index.go +++ b/pkg/sql/create_index.go @@ -182,7 +182,7 @@ func makeIndexDescriptor( } // Ensure that the index name does not exist before trying to create the index. - if idx, _ := tableDesc.FindIndexWithName(string(n.Name)); idx != nil { + if idx := catalog.FindIndexByName(tableDesc, string(n.Name)); idx != nil { if idx.Dropped() { return nil, pgerror.Newf(pgcode.DuplicateRelation, "index with name %q already exists and is being dropped, try again later", n.Name) } @@ -710,8 +710,8 @@ func maybeCreateAndAddShardCol( func (n *createIndexNode) startExec(params runParams) error { telemetry.Inc(sqltelemetry.SchemaChangeCreateCounter("index")) - foundIndex, err := n.tableDesc.FindIndexWithName(string(n.n.Name)) - if err == nil { + foundIndex := catalog.FindIndexByName(n.tableDesc, string(n.n.Name)) + if foundIndex != nil { if foundIndex.Dropped() { return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, "index %q being dropped, try again later", string(n.n.Name)) diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go index 73ad24b46273..65599c5a033c 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -1747,7 +1747,7 @@ func NewTableDesc( // indexes will be given a unique auto-generated name later on when // AllocateIDs is called. if d.Name != "" { - if idx, _ := desc.FindIndexWithName(d.Name.String()); idx != nil { + if idx := catalog.FindIndexByName(&desc, d.Name.String()); idx != nil { return nil, pgerror.Newf(pgcode.DuplicateRelation, "duplicate index name: %q", d.Name) } } @@ -1860,7 +1860,7 @@ func NewTableDesc( // indexes will be given a unique auto-generated name later on when // AllocateIDs is called. if d.Name != "" { - if idx, _ := desc.FindIndexWithName(d.Name.String()); idx != nil { + if idx := catalog.FindIndexByName(&desc, d.Name.String()); idx != nil { return nil, pgerror.Newf(pgcode.DuplicateRelation, "duplicate index name: %q", d.Name) } } diff --git a/pkg/sql/delete_preserving_index_test.go b/pkg/sql/delete_preserving_index_test.go index 3419b93b8183..b3d7e169a039 100644 --- a/pkg/sql/delete_preserving_index_test.go +++ b/pkg/sql/delete_preserving_index_test.go @@ -280,7 +280,7 @@ CREATE UNIQUE INDEX test_index_to_mutate ON t.test (b); _, err = sqlDB.Exec(`DELETE FROM t.test WHERE a = 2`) require.NoError(t, err) - idx, err := tableDesc.FindIndexWithName("test_index_to_mutate") + idx, err := catalog.MustFindIndexByName(tableDesc, "test_index_to_mutate") require.NoError(t, err) span := tableDesc.IndexSpan(codec, idx.GetID()) @@ -345,7 +345,7 @@ func mutateIndexByName( fn func(*descpb.IndexDescriptor) error, state descpb.DescriptorMutation_State, ) error { - idx, err := tableDesc.FindIndexWithName(index) + idx, err := catalog.MustFindIndexByName(tableDesc, index) if err != nil { return err } @@ -666,12 +666,12 @@ func TestMergeProcessor(t *testing.T) { tableDesc = desctestutils.TestingGetMutableExistingTableDescriptor(kvDB, codec, "d", "t") - dstIndex, err := tableDesc.FindIndexWithName(test.dstIndex) + dstIndex, err := catalog.MustFindIndexByName(tableDesc, test.dstIndex) if err != nil { t.Fatal(err) } - srcIndex, err := tableDesc.FindIndexWithName(test.srcIndex) + srcIndex, err := catalog.MustFindIndexByName(tableDesc, test.srcIndex) if err != nil { t.Fatal(err) } @@ -740,7 +740,7 @@ func fetchIndex( var alloc tree.DatumAlloc mm := mon.NewStandaloneBudget(1 << 30) - idx, err := table.FindIndexWithName(indexName) + idx, err := catalog.MustFindIndexByName(table, indexName) require.NoError(t, err) colIdxMap := catalog.ColumnIDToOrdinalMap(table.PublicColumns()) var valsNeeded intsets.Fast diff --git a/pkg/sql/descriptor_mutation_test.go b/pkg/sql/descriptor_mutation_test.go index 8e0a77ae421d..387cb811fc9b 100644 --- a/pkg/sql/descriptor_mutation_test.go +++ b/pkg/sql/descriptor_mutation_test.go @@ -21,6 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/sql" + "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/desctestutils" @@ -473,7 +474,7 @@ func (mt mutationTest) writeIndexMutation( ctx context.Context, index string, m descpb.DescriptorMutation, ) { tableDesc := mt.tableDesc - idx, err := tableDesc.FindIndexWithName(index) + idx, err := catalog.MustFindIndexByName(tableDesc, index) if err != nil { mt.Fatal(err) } diff --git a/pkg/sql/drop_index.go b/pkg/sql/drop_index.go index 073ca414e7b7..1ca81534a3c5 100644 --- a/pkg/sql/drop_index.go +++ b/pkg/sql/drop_index.go @@ -115,7 +115,7 @@ func (n *dropIndexNode) startExec(params runParams) error { // If we couldn't find the index by name, this is either a legitimate error or // this statement contains an 'IF EXISTS' qualifier. Both of these cases are // handled by `dropIndexByName()` below so we just ignore the error here. - idx, _ := tableDesc.FindIndexWithName(string(index.idxName)) + idx := catalog.FindIndexByName(tableDesc, string(index.idxName)) var shardColName string // If we're dropping a sharded index, record the name of its shard column to // potentially drop it if no other index refers to it. @@ -312,7 +312,7 @@ func (p *planner) dropIndexByName( constraintBehavior dropIndexConstraintBehavior, jobDesc string, ) error { - idx, err := tableDesc.FindIndexWithName(string(idxName)) + idx, err := catalog.MustFindIndexByName(tableDesc, string(idxName)) if err != nil { // Only index names of the form "table@idx" throw an error here if they // don't exist. diff --git a/pkg/sql/drop_test.go b/pkg/sql/drop_test.go index b5dbbd548ee2..8bad2ef0ea9f 100644 --- a/pkg/sql/drop_test.go +++ b/pkg/sql/drop_test.go @@ -432,7 +432,7 @@ func TestDropIndex(t *testing.T) { } tableDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "t", "kv") tests.CheckKeyCount(t, kvDB, tableDesc.TableSpan(keys.SystemSQLCodec), 3*numRows) - idx, err := tableDesc.FindIndexWithName("foo") + idx, err := catalog.MustFindIndexByName(tableDesc, "foo") if err != nil { t.Fatal(err) } @@ -443,7 +443,7 @@ func TestDropIndex(t *testing.T) { } tableDesc = desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "t", "kv") - if _, err := tableDesc.FindIndexWithName("foo"); err == nil { + if _, err := catalog.MustFindIndexByName(tableDesc, "foo"); err == nil { t.Fatalf("table descriptor still contains index after index is dropped") } // TODO (lucy): Maybe this test API should use an offset starting @@ -465,7 +465,7 @@ func TestDropIndex(t *testing.T) { } tableDesc = desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "t", "kv") - newIdx, err := tableDesc.FindIndexWithName("foo") + newIdx, err := catalog.MustFindIndexByName(tableDesc, "foo") if err != nil { t.Fatal(err) } @@ -528,7 +528,7 @@ func TestDropIndexWithZoneConfigOSS(t *testing.T) { t.Fatal(err) } tableDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "t", "kv") - index, err := tableDesc.FindIndexWithName("foo") + index, err := catalog.MustFindIndexByName(tableDesc, "foo") if err != nil { t.Fatal(err) } @@ -567,7 +567,7 @@ func TestDropIndexWithZoneConfigOSS(t *testing.T) { // declares column families. tableDesc = desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "t", "kv") - if _, err := tableDesc.FindIndexWithName("foo"); err == nil { + if _, err := catalog.MustFindIndexByName(tableDesc, "foo"); err == nil { t.Fatalf("table descriptor still contains index after index is dropped") } } @@ -1216,7 +1216,7 @@ func TestDropIndexOnHashShardedIndexWithStoredShardColumn(t *testing.T) { tableDesc, err = col.ByID(txn.KV()).Get().Table(ctx, tableID) return err })) - shardIdx, err := tableDesc.FindIndexWithName("idx") + shardIdx, err := catalog.MustFindIndexByName(tableDesc, "idx") require.NoError(t, err) require.True(t, shardIdx.IsSharded()) require.Equal(t, "crdb_internal_a_shard_7", shardIdx.GetShardColumnName()) @@ -1234,7 +1234,7 @@ func TestDropIndexOnHashShardedIndexWithStoredShardColumn(t *testing.T) { tableDesc, err = col.ByID(txn.KV()).Get().Table(ctx, tableID) return err })) - _, err = tableDesc.FindIndexWithName("idx") + _, err = catalog.MustFindIndexByName(tableDesc, "idx") require.Error(t, err) shardCol, err = tableDesc.FindColumnWithName("crdb_internal_a_shard_7") require.NoError(t, err) diff --git a/pkg/sql/evalcatalog/encode_table_index_key.go b/pkg/sql/evalcatalog/encode_table_index_key.go index 528d59ca9dae..82aaa0aed584 100644 --- a/pkg/sql/evalcatalog/encode_table_index_key.go +++ b/pkg/sql/evalcatalog/encode_table_index_key.go @@ -37,7 +37,7 @@ func (ec *Builtins) EncodeTableIndexKey( if err != nil { return nil, err } - index, err := tableDesc.FindIndexWithID(indexID) + index, err := catalog.MustFindIndexByID(tableDesc, indexID) if err != nil { return nil, err } diff --git a/pkg/sql/evalcatalog/geo_inverted_index_entries.go b/pkg/sql/evalcatalog/geo_inverted_index_entries.go index f979857e05a2..2e5d474e7670 100644 --- a/pkg/sql/evalcatalog/geo_inverted_index_entries.go +++ b/pkg/sql/evalcatalog/geo_inverted_index_entries.go @@ -15,6 +15,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/geo/geoindex" "github.com/cockroachdb/cockroach/pkg/kv" + "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" @@ -78,7 +79,7 @@ func getIndexGeoConfig( if err != nil { return geoindex.Config{}, err } - index, err := tableDesc.FindIndexWithID(indexID) + index, err := catalog.MustFindIndexByID(tableDesc, indexID) if err != nil { return geoindex.Config{}, err } diff --git a/pkg/sql/indexbackfiller_test.go b/pkg/sql/indexbackfiller_test.go index 82c272e19939..83c584115771 100644 --- a/pkg/sql/indexbackfiller_test.go +++ b/pkg/sql/indexbackfiller_test.go @@ -370,7 +370,7 @@ INSERT INTO foo VALUES (1), (10), (100); t.Helper() mm := mon.NewStandaloneBudget(1 << 30) - idx, err := table.FindIndexWithID(indexID) + idx, err := catalog.MustFindIndexByID(table, indexID) colIDsNeeded := idx.CollectKeyColumnIDs() if idx.Primary() { for _, column := range table.PublicColumns() { diff --git a/pkg/sql/rename_index.go b/pkg/sql/rename_index.go index 206dfd40b571..0d64efe9e292 100644 --- a/pkg/sql/rename_index.go +++ b/pkg/sql/rename_index.go @@ -53,7 +53,7 @@ func (p *planner) RenameIndex(ctx context.Context, n *tree.RenameIndex) (planNod return newZeroNode(nil /* columns */), nil } - idx, err := tableDesc.FindIndexWithName(string(n.Index.Index)) + idx, err := catalog.MustFindIndexByName(tableDesc, string(n.Index.Index)) if err != nil { if n.IfExists { // Noop. @@ -99,7 +99,7 @@ func (n *renameIndexNode) startExec(params runParams) error { return nil } - if foundIndex, _ := tableDesc.FindIndexWithName(string(n.n.NewName)); foundIndex != nil { + if foundIndex := catalog.FindIndexByName(tableDesc, string(n.n.NewName)); foundIndex != nil { return pgerror.Newf(pgcode.DuplicateRelation, "index name %q already exists", string(n.n.NewName)) } diff --git a/pkg/sql/resolver.go b/pkg/sql/resolver.go index 735ce50670b2..3ffbb692bf5f 100644 --- a/pkg/sql/resolver.go +++ b/pkg/sql/resolver.go @@ -751,7 +751,7 @@ func (p *planner) getTableAndIndexImpl( return catalog.ResolvedObjectPrefix{}, nil, nil, errors.NewAssertionErrorWithWrappedErrf(err, "failed to re-resolve table %d for index %s", tbl.GetID(), tableWithIndex) } - retIdx, err := mut.FindIndexWithID(idx.GetID()) + retIdx, err := catalog.MustFindIndexByID(mut, idx.GetID()) if err != nil { return catalog.ResolvedObjectPrefix{}, nil, nil, errors.NewAssertionErrorWithWrappedErrf(err, "retrieving index %s (%d) from table which was known to already exist for table %d", diff --git a/pkg/sql/row/errors.go b/pkg/sql/row/errors.go index 103cefdf986d..c2b52fe02638 100644 --- a/pkg/sql/row/errors.go +++ b/pkg/sql/row/errors.go @@ -196,7 +196,7 @@ func decodeKeyCodecAndIndex( if err != nil { return keys.SQLCodec{}, nil, err } - index, err := tableDesc.FindIndexWithID(indexID) + index, err := catalog.MustFindIndexByID(tableDesc, indexID) if err != nil { return keys.SQLCodec{}, nil, err } diff --git a/pkg/sql/rowenc/index_encoding_test.go b/pkg/sql/rowenc/index_encoding_test.go index b97c55d1baa4..753bf513b00f 100644 --- a/pkg/sql/rowenc/index_encoding_test.go +++ b/pkg/sql/rowenc/index_encoding_test.go @@ -657,7 +657,7 @@ func ExtractIndexKey( return entry.Key, nil } - index, err := tableDesc.FindIndexWithID(indexID) + index, err := catalog.MustFindIndexByID(tableDesc, indexID) if err != nil { return nil, err } diff --git a/pkg/sql/rowenc/index_fetch_test.go b/pkg/sql/rowenc/index_fetch_test.go index 19fc5b15174a..922bb738e6bf 100644 --- a/pkg/sql/rowenc/index_fetch_test.go +++ b/pkg/sql/rowenc/index_fetch_test.go @@ -17,6 +17,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" "github.com/cockroachdb/cockroach/pkg/sql/catalog/fetchpb" @@ -59,7 +60,7 @@ func TestInitIndexFetchSpec(t *testing.T) { d.Fatalf(t, "failed to parse index-fetch params: %v", err) } table := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "testdb", params.Table) - index, err := table.FindIndexWithName(params.Index) + index, err := catalog.MustFindIndexByName(table, params.Index) if err != nil { d.Fatalf(t, "%+v", err) } diff --git a/pkg/sql/rowexec/utils_test.go b/pkg/sql/rowexec/utils_test.go index 3b26be455c2f..5e819397278e 100644 --- a/pkg/sql/rowexec/utils_test.go +++ b/pkg/sql/rowexec/utils_test.go @@ -201,7 +201,7 @@ func (r *rowDisposer) NumRowsDisposed() int { func makeFetchSpec( t testing.TB, table catalog.TableDescriptor, indexName string, colNames string, ) fetchpb.IndexFetchSpec { - index, err := table.FindIndexWithName(indexName) + index, err := catalog.MustFindIndexByName(table, indexName) if err != nil { t.Fatal(err) } diff --git a/pkg/sql/schema_changer_test.go b/pkg/sql/schema_changer_test.go index 235c470008ba..60049832e9a9 100644 --- a/pkg/sql/schema_changer_test.go +++ b/pkg/sql/schema_changer_test.go @@ -6478,7 +6478,7 @@ CREATE INDEX i ON t.test (a) WHERE b > 2 } tableDesc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "t", "test") - index, err := tableDesc.FindIndexWithName("i") + index, err := catalog.MustFindIndexByName(tableDesc, "i") if err != nil { t.Fatalf("unexpected error: %s", err) } diff --git a/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go b/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go index 52c40968c58a..d4f07d8e0983 100644 --- a/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go +++ b/pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go @@ -324,7 +324,13 @@ func (s *TestState) MayResolveIndex( if tbl == nil { return false, catalog.ResolvedObjectPrefix{}, nil, nil } - idx, _ = tbl.FindNonDropIndexWithName(string(tableIndexName.Index)) + idx = catalog.FindIndex( + tbl, + catalog.IndexOpts{AddMutations: true}, + func(idx catalog.Index) bool { + return idx.GetName() == string(tableIndexName.Index) + }, + ) } else { db, schema := s.mayResolvePrefix(tableIndexName.Table.ObjectNamePrefix) prefix = catalog.ResolvedObjectPrefix{ @@ -343,7 +349,13 @@ func (s *TestState) MayResolveIndex( if !ok { return nil } - idx, _ = tbl.FindNonDropIndexWithName(string(tableIndexName.Index)) + idx = catalog.FindIndex( + tbl, + catalog.IndexOpts{AddMutations: true}, + func(idx catalog.Index) bool { + return idx.GetName() == string(tableIndexName.Index) + }, + ) if idx != nil { return iterutil.StopIteration() } diff --git a/pkg/sql/schemachanger/scexec/exec_backfill_test.go b/pkg/sql/schemachanger/scexec/exec_backfill_test.go index d6f0bb2a77aa..cf2d27b5ebf3 100644 --- a/pkg/sql/schemachanger/scexec/exec_backfill_test.go +++ b/pkg/sql/schemachanger/scexec/exec_backfill_test.go @@ -93,7 +93,7 @@ func TestExecBackfiller(t *testing.T) { EncodingType: catenumpb.SecondaryIndexEncoding, UseDeletePreservingEncoding: isTempIndex, }, descpb.DescriptorMutation_ADD, descpb.DescriptorMutation_BACKFILLING)) - idx, err := mut.FindIndexWithName(name) + idx, err := catalog.MustFindIndexByName(mut, name) require.NoError(t, err) return idx } diff --git a/pkg/sql/schemachanger/scexec/exec_validation.go b/pkg/sql/schemachanger/scexec/exec_validation.go index 92b06a13544f..3db435b27f48 100644 --- a/pkg/sql/schemachanger/scexec/exec_validation.go +++ b/pkg/sql/schemachanger/scexec/exec_validation.go @@ -33,7 +33,7 @@ func executeValidateUniqueIndex( if !ok { return catalog.WrapTableDescRefErr(desc.GetID(), catalog.NewDescriptorTypeError(desc)) } - index, err := table.FindIndexWithID(op.IndexID) + index, err := catalog.MustFindIndexByID(table, op.IndexID) if err != nil { return err } diff --git a/pkg/sql/schemachanger/scexec/scmutationexec/eventlog.go b/pkg/sql/schemachanger/scexec/scmutationexec/eventlog.go index 0f1713ea9f32..73b7e705649f 100644 --- a/pkg/sql/schemachanger/scexec/scmutationexec/eventlog.go +++ b/pkg/sql/schemachanger/scexec/scmutationexec/eventlog.go @@ -13,6 +13,7 @@ package scmutationexec import ( "context" + "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scop" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/screl" @@ -166,7 +167,7 @@ func asCommentEventPayload( if err != nil { return nil, err } - idx, err := tbl.FindIndexWithID(e.IndexID) + idx, err := catalog.MustFindIndexByID(tbl, e.IndexID) if err != nil { return nil, err } diff --git a/pkg/sql/schemachanger/scexec/scmutationexec/index.go b/pkg/sql/schemachanger/scexec/scmutationexec/index.go index a08476159f28..f500de0e7e23 100644 --- a/pkg/sql/schemachanger/scexec/scmutationexec/index.go +++ b/pkg/sql/schemachanger/scexec/scmutationexec/index.go @@ -190,7 +190,7 @@ func (m *visitor) MakeValidatedPrimaryIndexPublic( if err != nil { return err } - index, err := tbl.FindIndexWithID(op.IndexID) + index, err := catalog.MustFindIndexByID(tbl, op.IndexID) if err != nil { return err } @@ -275,7 +275,7 @@ func (m *visitor) MakeWriteOnlyIndexDeleteOnly( if err != nil { return err } - idx, err := tbl.FindIndexWithID(op.IndexID) + idx, err := catalog.MustFindIndexByID(tbl, op.IndexID) if err != nil { return err } @@ -329,7 +329,7 @@ func (m *visitor) AddIndexPartitionInfo(ctx context.Context, op scop.AddIndexPar if err != nil { return err } - index, err := tbl.FindIndexWithID(op.Partitioning.IndexID) + index, err := catalog.MustFindIndexByID(tbl, op.Partitioning.IndexID) if err != nil { return err } @@ -342,7 +342,7 @@ func (m *visitor) SetIndexName(ctx context.Context, op scop.SetIndexName) error if err != nil { return err } - index, err := tbl.FindIndexWithID(op.IndexID) + index, err := catalog.MustFindIndexByID(tbl, op.IndexID) if err != nil { return err } @@ -355,7 +355,7 @@ func (m *visitor) AddColumnToIndex(ctx context.Context, op scop.AddColumnToIndex if err != nil { return err } - index, err := tbl.FindIndexWithID(op.IndexID) + index, err := catalog.MustFindIndexByID(tbl, op.IndexID) if err != nil { return err } @@ -426,7 +426,7 @@ func (m *visitor) RemoveColumnFromIndex(ctx context.Context, op scop.RemoveColum if err != nil { return err } - index, err := tbl.FindIndexWithID(op.IndexID) + index, err := catalog.MustFindIndexByID(tbl, op.IndexID) if err != nil { return err } diff --git a/pkg/sql/show_create_clauses.go b/pkg/sql/show_create_clauses.go index 6ac6b9097c5f..ef71a570a053 100644 --- a/pkg/sql/show_create_clauses.go +++ b/pkg/sql/show_create_clauses.go @@ -393,7 +393,7 @@ func showComments( } for _, indexComment := range tc.indexes { - idx, err := table.FindIndexWithID(descpb.IndexID(indexComment.subID)) + idx, err := catalog.MustFindIndexByID(table, descpb.IndexID(indexComment.subID)) if err != nil { return err } diff --git a/pkg/sql/span/BUILD.bazel b/pkg/sql/span/BUILD.bazel index 1185fd365a3a..5bedba26edfe 100644 --- a/pkg/sql/span/BUILD.bazel +++ b/pkg/sql/span/BUILD.bazel @@ -44,6 +44,7 @@ go_test( "//pkg/security/securityassets", "//pkg/security/securitytest", "//pkg/server", + "//pkg/sql/catalog", "//pkg/sql/catalog/desctestutils", "//pkg/sql/catalog/systemschema", "//pkg/sql/tests", diff --git a/pkg/sql/span/span_splitter_test.go b/pkg/sql/span/span_splitter_test.go index 9e3248242d2e..2794c5ebf595 100644 --- a/pkg/sql/span/span_splitter_test.go +++ b/pkg/sql/span/span_splitter_test.go @@ -16,6 +16,7 @@ import ( "testing" "github.com/cockroachdb/cockroach/pkg/keys" + "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" "github.com/cockroachdb/cockroach/pkg/sql/span" @@ -127,7 +128,7 @@ func TestSpanSplitterCanSplitSpan(t *testing.T) { t.Fatal(err) } desc := desctestutils.TestingGetPublicTableDescriptor(kvDB, keys.SystemSQLCodec, "t", "t") - idx, err := desc.FindIndexWithName(tc.index) + idx, err := catalog.MustFindIndexByName(desc, tc.index) if err != nil { t.Fatal(err) } diff --git a/pkg/sql/tests/hash_sharded_test.go b/pkg/sql/tests/hash_sharded_test.go index 5077572beb3d..099d38980986 100644 --- a/pkg/sql/tests/hash_sharded_test.go +++ b/pkg/sql/tests/hash_sharded_test.go @@ -30,7 +30,7 @@ import ( func getShardColumnID( t *testing.T, tableDesc catalog.TableDescriptor, shardedIndexName string, ) descpb.ColumnID { - idx, err := tableDesc.FindIndexWithName(shardedIndexName) + idx, err := catalog.MustFindIndexByName(tableDesc, shardedIndexName) if err != nil { t.Fatal(err) } @@ -49,7 +49,7 @@ func getShardColumnID( func verifyTableDescriptorState( t *testing.T, tableDesc catalog.TableDescriptor, shardedIndexName string, ) { - idx, err := tableDesc.FindIndexWithName(shardedIndexName) + idx, err := catalog.MustFindIndexByName(tableDesc, shardedIndexName) if err != nil { t.Fatal(err) } @@ -106,7 +106,7 @@ func TestBasicHashShardedIndexes(t *testing.T) { // Ensure that secondary indexes on table `kv` have the shard column in their // `KeySuffixColumnIDs` field so they can reconstruct the sharded primary key. - foo, err := tableDesc.FindIndexWithName("foo") + foo, err := catalog.MustFindIndexByName(tableDesc, "foo") if err != nil { t.Fatal(err) } diff --git a/pkg/sql/unsplit_range_test.go b/pkg/sql/unsplit_range_test.go index a1fb5267cd5c..17eb76e271f4 100644 --- a/pkg/sql/unsplit_range_test.go +++ b/pkg/sql/unsplit_range_test.go @@ -292,7 +292,7 @@ func TestUnsplitRanges(t *testing.T) { tableSpan := tableDesc.TableSpan(keys.SystemSQLCodec) tests.CheckKeyCount(t, kvDB, tableSpan, numKeys) - idx, err := tableDesc.FindIndexWithName("foo") + idx, err := catalog.MustFindIndexByName(tableDesc, "foo") if err != nil { t.Fatal(err) } diff --git a/pkg/sql/zone_config.go b/pkg/sql/zone_config.go index e5eb62577864..2f024cb5e0d4 100644 --- a/pkg/sql/zone_config.go +++ b/pkg/sql/zone_config.go @@ -449,7 +449,7 @@ func resolveSubzone( indexName = index.GetName() } else { var err error - index, err = table.FindIndexWithName(indexName) + index, err = catalog.MustFindIndexByName(table, indexName) if err != nil { return nil, "", err } diff --git a/pkg/upgrade/upgrades/sampled_stmt_diagnostics_requests.go b/pkg/upgrade/upgrades/sampled_stmt_diagnostics_requests.go index 21b4007d675b..03c632df806f 100644 --- a/pkg/upgrade/upgrades/sampled_stmt_diagnostics_requests.go +++ b/pkg/upgrade/upgrades/sampled_stmt_diagnostics_requests.go @@ -57,7 +57,7 @@ func sampledStmtDiagReqsMigration( // We want to determine whether the old index from 21.2 exists. // That index has one stored column. The index we introduce below has // four. - idx, _ := existing.FindIndexWithName("completed_idx") + idx := catalog.FindIndexByName(existing, "completed_idx") // If the index does not exist, we're good to proceed. if idx == nil { return true, nil diff --git a/pkg/upgrade/upgrades/schema_changes.go b/pkg/upgrade/upgrades/schema_changes.go index 25b69170a32c..8e0289fdef0f 100644 --- a/pkg/upgrade/upgrades/schema_changes.go +++ b/pkg/upgrade/upgrades/schema_changes.go @@ -273,14 +273,11 @@ func columnExistsAndIsNotNull( // expectedTable descriptor. The comparison is not strict as several descriptor // fields are ignored. func hasIndex(storedTable, expectedTable catalog.TableDescriptor, indexName string) (bool, error) { - storedIdx, err := storedTable.FindIndexWithName(indexName) - if err != nil { - if strings.Contains(err.Error(), "does not exist") { - return false, nil - } - return false, err + storedIdx := catalog.FindIndexByName(storedTable, indexName) + if storedIdx == nil { + return false, nil } - expectedIdx, err := expectedTable.FindIndexWithName(indexName) + expectedIdx, err := catalog.MustFindIndexByName(expectedTable, indexName) if err != nil { return false, errors.Wrapf(err, "index name %s is invalid", indexName) } @@ -327,7 +324,7 @@ func indexDescForComparison(idx catalog.Index) *descpb.IndexDescriptor { func doesNotHaveIndex( storedTable, expectedTable catalog.TableDescriptor, indexName string, ) (bool, error) { - idx, _ := storedTable.FindIndexWithName(indexName) + idx := catalog.FindIndexByName(storedTable, indexName) return idx == nil, nil } From 3188422805d6dc65aa31ed09cf2a3e77647f379b Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Wed, 11 Jan 2023 10:23:11 -0500 Subject: [PATCH 6/9] catalog: replace FindConstraintWith* methods with functions This commit refactors these methods as functions. Informs #91924. Release note: None --- pkg/sql/alter_table.go | 10 ++-- pkg/sql/catalog/descriptor.go | 7 --- pkg/sql/catalog/funcdesc/func_desc.go | 3 +- pkg/sql/catalog/table_elements.go | 50 ++++++++++++++++++- pkg/sql/catalog/tabledesc/structured_test.go | 4 +- pkg/sql/catalog/tabledesc/table.go | 22 -------- pkg/sql/check.go | 2 +- pkg/sql/comment_on_constraint.go | 2 +- pkg/sql/create_table.go | 12 ++--- pkg/sql/pg_catalog.go | 2 +- .../schemachanger/scexec/exec_validation.go | 2 +- .../scexec/scmutationexec/constraint.go | 3 +- .../scexec/scmutationexec/eventlog.go | 4 +- pkg/sql/scrub.go | 2 +- pkg/sql/show_create_clauses.go | 2 +- 15 files changed, 72 insertions(+), 55 deletions(-) diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index 74c7bc2c8621..0ee414fd68de 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -499,7 +499,7 @@ func (n *alterTableNode) startExec(params runParams) error { droppedViews = append(droppedViews, colDroppedViews...) case *tree.AlterTableDropConstraint: name := string(t.Constraint) - c, _ := n.tableDesc.FindConstraintWithName(name) + c := catalog.FindConstraintByName(n.tableDesc, name) if c == nil { if t.IfExists { continue @@ -519,7 +519,7 @@ func (n *alterTableNode) startExec(params runParams) error { case *tree.AlterTableValidateConstraint: name := string(t.Constraint) - c, _ := n.tableDesc.FindConstraintWithName(name) + c := catalog.FindConstraintByName(n.tableDesc, name) if c == nil { return pgerror.Newf(pgcode.UndefinedObject, "constraint %q of relation %q does not exist", t.Constraint, n.tableDesc.Name) @@ -743,7 +743,7 @@ func (n *alterTableNode) startExec(params runParams) error { descriptorChanged = descriptorChanged || descChanged case *tree.AlterTableRenameConstraint: - constraint, _ := n.tableDesc.FindConstraintWithName(string(t.Constraint)) + constraint := catalog.FindConstraintByName(n.tableDesc, string(t.Constraint)) if constraint == nil { return pgerror.Newf(pgcode.UndefinedObject, "constraint %q of relation %q does not exist", tree.ErrString(&t.Constraint), n.tableDesc.Name) @@ -760,7 +760,7 @@ func (n *alterTableNode) startExec(params runParams) error { return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, "constraint %q in the middle of being dropped", t.Constraint) } - if other, _ := n.tableDesc.FindConstraintWithName(string(t.NewName)); other != nil { + if other := catalog.FindConstraintByName(n.tableDesc, string(t.NewName)); other != nil { return pgerror.Newf(pgcode.DuplicateObject, "duplicate constraint name: %q", tree.ErrString(&t.NewName)) } @@ -1451,7 +1451,7 @@ func validateConstraintNameIsNotUsed( if name == "" { return false, nil } - constraint, _ := tableDesc.FindConstraintWithName(string(name)) + constraint := catalog.FindConstraintByName(tableDesc, string(name)) if constraint == nil { return false, nil } diff --git a/pkg/sql/catalog/descriptor.go b/pkg/sql/catalog/descriptor.go index 2be6eefb0981..44742d7ee2be 100644 --- a/pkg/sql/catalog/descriptor.go +++ b/pkg/sql/catalog/descriptor.go @@ -656,13 +656,6 @@ type TableDescriptor interface { // in the same order. EnforcedUniqueConstraintsWithoutIndex() []UniqueWithoutIndexConstraint - // FindConstraintWithID traverses the slice returned by AllConstraints and - // returns the first catalog.Constraint that matches the desired ID, or an - // error if none was found. - FindConstraintWithID(id descpb.ConstraintID) (Constraint, error) - // FindConstraintWithName is like FindConstraintWithID but for names. - FindConstraintWithName(name string) (Constraint, error) - // CheckConstraintColumns returns the slice of columns referenced by a check // constraint. CheckConstraintColumns(ck CheckConstraint) []Column diff --git a/pkg/sql/catalog/funcdesc/func_desc.go b/pkg/sql/catalog/funcdesc/func_desc.go index 3834293084ba..83a03e968a58 100644 --- a/pkg/sql/catalog/funcdesc/func_desc.go +++ b/pkg/sql/catalog/funcdesc/func_desc.go @@ -328,8 +328,7 @@ func (desc *immutable) validateInboundTableRef( } for _, cstID := range by.ConstraintIDs { - _, err := backRefTbl.FindConstraintWithID(cstID) - if err != nil { + if catalog.FindConstraintByID(backRefTbl, cstID) == nil { return errors.AssertionFailedf("depended-on-by relation %q (%d) does not have a constraint with ID %d", backRefTbl.GetName(), by.ID, cstID) } diff --git a/pkg/sql/catalog/table_elements.go b/pkg/sql/catalog/table_elements.go index 66446dbe84d6..ae8c70c341a8 100644 --- a/pkg/sql/catalog/table_elements.go +++ b/pkg/sql/catalog/table_elements.go @@ -18,6 +18,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/catenumpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" "github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants" "github.com/cockroachdb/cockroach/pkg/sql/sem/semenumpb" @@ -1081,7 +1083,7 @@ func FindIndexByID(tbl TableDescriptor, id descpb.IndexID) Index { }) } -// MustFindIndexByID is like FindIndexByID but returns an error when no index +// MustFindIndexByID is like FindIndexByID but returns an error when no Index // was found. func MustFindIndexByID(tbl TableDescriptor, id descpb.IndexID) (Index, error) { if idx := FindIndexByID(tbl, id); idx != nil { @@ -1108,3 +1110,49 @@ func MustFindIndexByName(tbl TableDescriptor, name string) (Index, error) { } return nil, errors.Errorf("index %q does not exist", name) } + +// FindConstraintByID traverses the slice returned by the AllConstraints +// method on the table descriptor and returns the first Constraint that +// matches the desired ID, or nil if none was found. +func FindConstraintByID(tbl TableDescriptor, id descpb.ConstraintID) Constraint { + all := tbl.AllConstraints() + for _, c := range all { + if c.GetConstraintID() == id { + return c + } + } + return nil +} + +// MustFindConstraintByID is like FindConstraintByID but returns an error when +// no Constraint was found. +func MustFindConstraintByID(tbl TableDescriptor, id descpb.ConstraintID) (Constraint, error) { + if c := FindConstraintByID(tbl, id); c != nil { + return c, nil + } + return nil, pgerror.Newf(pgcode.UndefinedObject, "constraint-id \"%d\" does not exist", id) +} + +// FindConstraintByName is like FindConstraintByID but with names instead of +// IDs. +func FindConstraintByName(tbl TableDescriptor, name string) Constraint { + all := tbl.AllConstraints() + for _, c := range all { + if c.GetName() == name { + return c + } + } + return nil +} + +// MustFindConstraintWithName is like MustFindConstraintByID but with names +// instead of IDs. +func MustFindConstraintWithName(tbl TableDescriptor, name string) (Constraint, error) { + if c := FindConstraintByName(tbl, name); c != nil { + return c, nil + } + return nil, pgerror.Newf(pgcode.UndefinedObject, "constraint named %q does not exist", name) +} + +// silence the linter +var _ = MustFindConstraintWithName diff --git a/pkg/sql/catalog/tabledesc/structured_test.go b/pkg/sql/catalog/tabledesc/structured_test.go index aec8b55e6372..987ee1fcff7b 100644 --- a/pkg/sql/catalog/tabledesc/structured_test.go +++ b/pkg/sql/catalog/tabledesc/structured_test.go @@ -661,13 +661,13 @@ func TestUnvalidateConstraints(t *testing.T) { t.Fatal(err) } - before, _ := desc.FindConstraintWithName("fk") + before := catalog.FindConstraintByName(desc, "fk") if before == nil || !before.IsConstraintValidated() { t.Fatalf("expected to find a validated constraint fk before, found %v", before) } desc.InvalidateFKConstraints() - after, _ := desc.FindConstraintWithName("fk") + after := catalog.FindConstraintByName(desc, "fk") if after == nil || before.IsConstraintValidated() { t.Fatalf("expected to find an unvalidated constraint fk before, found %v", after) } diff --git a/pkg/sql/catalog/tabledesc/table.go b/pkg/sql/catalog/tabledesc/table.go index 518129f91e4d..ac5c0da33308 100644 --- a/pkg/sql/catalog/tabledesc/table.go +++ b/pkg/sql/catalog/tabledesc/table.go @@ -323,28 +323,6 @@ func (desc *wrapper) getExistingOrNewConstraintCache() *constraintCache { return newConstraintCache(desc.TableDesc(), desc.getExistingOrNewIndexCache(), desc.getExistingOrNewMutationCache()) } -// FindConstraintWithID implements the TableDescriptor interface. -func (desc *wrapper) FindConstraintWithID(id descpb.ConstraintID) (catalog.Constraint, error) { - all := desc.AllConstraints() - for _, c := range all { - if c.GetConstraintID() == id { - return c, nil - } - } - return nil, pgerror.Newf(pgcode.UndefinedObject, "constraint-id \"%d\" does not exist", id) -} - -// FindConstraintWithName implements the TableDescriptor interface. -func (desc *wrapper) FindConstraintWithName(name string) (catalog.Constraint, error) { - all := desc.AllConstraints() - for _, c := range all { - if c.GetName() == name { - return c, nil - } - } - return nil, pgerror.Newf(pgcode.UndefinedObject, "constraint named %q does not exist", name) -} - // AllConstraints implements the catalog.TableDescriptor interface. func (desc *wrapper) AllConstraints() []catalog.Constraint { return desc.getExistingOrNewConstraintCache().all diff --git a/pkg/sql/check.go b/pkg/sql/check.go index 89c31713304c..cc98f76851bd 100644 --- a/pkg/sql/check.go +++ b/pkg/sql/check.go @@ -528,7 +528,7 @@ func (p *planner) IsConstraintActive( if err != nil { return false, err } - constraint, _ := tableDesc.FindConstraintWithName(constraintName) + constraint := catalog.FindConstraintByName(tableDesc, constraintName) return constraint != nil && constraint.IsEnforced(), nil } diff --git a/pkg/sql/comment_on_constraint.go b/pkg/sql/comment_on_constraint.go index 263682eaff0a..7adde074b358 100644 --- a/pkg/sql/comment_on_constraint.go +++ b/pkg/sql/comment_on_constraint.go @@ -58,7 +58,7 @@ func (p *planner) CommentOnConstraint( func (n *commentOnConstraintNode) startExec(params runParams) error { constraintName := string(n.n.Constraint) - constraint, _ := n.tableDesc.FindConstraintWithName(constraintName) + constraint := catalog.FindConstraintByName(n.tableDesc, constraintName) if constraint == nil { return pgerror.Newf(pgcode.UndefinedObject, "constraint %q of relation %q does not exist", constraintName, n.tableDesc.GetName()) diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go index 65599c5a033c..9b709dbb793a 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -756,12 +756,11 @@ func ResolveUniqueWithoutIndexConstraint( constraintName = tabledesc.GenerateUniqueName( fmt.Sprintf("unique_%s", strings.Join(colNames, "_")), func(p string) bool { - c, _ := tbl.FindConstraintWithName(p) - return c != nil + return catalog.FindConstraintByName(tbl, p) != nil }, ) } else { - if c, _ := tbl.FindConstraintWithName(constraintName); c != nil { + if c := catalog.FindConstraintByName(tbl, constraintName); c != nil { return pgerror.Newf(pgcode.DuplicateObject, "duplicate constraint name: %q", constraintName) } } @@ -963,12 +962,11 @@ func ResolveFK( constraintName = tabledesc.GenerateUniqueName( tabledesc.ForeignKeyConstraintName(tbl.GetName(), d.FromCols.ToStrings()), func(p string) bool { - c, _ := tbl.FindConstraintWithName(p) - return c != nil + return catalog.FindConstraintByName(tbl, p) != nil }, ) } else { - if c, _ := tbl.FindConstraintWithName(constraintName); c != nil { + if c := catalog.FindConstraintByName(tbl, constraintName); c != nil { return pgerror.Newf(pgcode.DuplicateObject, "duplicate constraint name: %q", constraintName) } } @@ -1041,7 +1039,7 @@ func ResolveFK( tbl.AddForeignKeyMutation(&ref, descpb.DescriptorMutation_ADD) } - c, err := tbl.FindConstraintWithID(ref.ConstraintID) + c, err := catalog.MustFindConstraintByID(tbl, ref.ConstraintID) if err != nil { return errors.HandleAsAssertionFailure(err) } diff --git a/pkg/sql/pg_catalog.go b/pkg/sql/pg_catalog.go index 7f588ec3f028..1899e951afa2 100644 --- a/pkg/sql/pg_catalog.go +++ b/pkg/sql/pg_catalog.go @@ -1607,7 +1607,7 @@ https://www.postgresql.org/docs/9.5/catalog-pg-description.html`, if err != nil { return err } - c, err := tableDesc.FindConstraintWithID(descpb.ConstraintID(tree.MustBeDInt(objSubID))) + c, err := catalog.MustFindConstraintByID(tableDesc, descpb.ConstraintID(tree.MustBeDInt(objSubID))) if err != nil { return err } diff --git a/pkg/sql/schemachanger/scexec/exec_validation.go b/pkg/sql/schemachanger/scexec/exec_validation.go index 3db435b27f48..7abf5adef1e7 100644 --- a/pkg/sql/schemachanger/scexec/exec_validation.go +++ b/pkg/sql/schemachanger/scexec/exec_validation.go @@ -62,7 +62,7 @@ func executeValidateConstraint( if err != nil { return err } - constraint, err := table.FindConstraintWithID(op.ConstraintID) + constraint, err := catalog.MustFindConstraintByID(table, op.ConstraintID) if err != nil { return err } diff --git a/pkg/sql/schemachanger/scexec/scmutationexec/constraint.go b/pkg/sql/schemachanger/scexec/scmutationexec/constraint.go index 34880529d12e..baa618f266f1 100644 --- a/pkg/sql/schemachanger/scexec/scmutationexec/constraint.go +++ b/pkg/sql/schemachanger/scexec/scmutationexec/constraint.go @@ -13,6 +13,7 @@ package scmutationexec import ( "context" + "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scop" @@ -24,7 +25,7 @@ func (m *visitor) SetConstraintName(ctx context.Context, op scop.SetConstraintNa if err != nil { return err } - constraint, err := tbl.FindConstraintWithID(op.ConstraintID) + constraint, err := catalog.MustFindConstraintByID(tbl, op.ConstraintID) if err != nil { return err } diff --git a/pkg/sql/schemachanger/scexec/scmutationexec/eventlog.go b/pkg/sql/schemachanger/scexec/scmutationexec/eventlog.go index 73b7e705649f..da8ebf203237 100644 --- a/pkg/sql/schemachanger/scexec/scmutationexec/eventlog.go +++ b/pkg/sql/schemachanger/scexec/scmutationexec/eventlog.go @@ -183,8 +183,8 @@ func asCommentEventPayload( return nil, err } var constraintName string - if constraint, err := tbl.FindConstraintWithID(e.ConstraintID); err != nil { - // FindConstraintWithID excludes dropping indexes for no good reason. + if constraint, err := catalog.MustFindConstraintByID(tbl, e.ConstraintID); err != nil { + // MustFindConstraintByID excludes dropping indexes for no good reason. // TODO(postamar): improve catalog.TableDescriptor interface for _, idx := range tbl.AllIndexes() { if idx.Dropped() && idx.GetConstraintID() == e.ConstraintID { diff --git a/pkg/sql/scrub.go b/pkg/sql/scrub.go index de2dfc798f1d..ca1d1927ebb7 100644 --- a/pkg/sql/scrub.go +++ b/pkg/sql/scrub.go @@ -419,7 +419,7 @@ func createConstraintCheckOperations( // constraintNames. if constraintNames != nil { for _, constraintName := range constraintNames { - c, _ := tableDesc.FindConstraintWithName(string(constraintName)) + c := catalog.FindConstraintByName(tableDesc, string(constraintName)) if c == nil { return nil, pgerror.Newf(pgcode.UndefinedObject, "constraint %q of relation %q does not exist", constraintName, tableDesc.GetName()) diff --git a/pkg/sql/show_create_clauses.go b/pkg/sql/show_create_clauses.go index ef71a570a053..3087d2bf229a 100644 --- a/pkg/sql/show_create_clauses.go +++ b/pkg/sql/show_create_clauses.go @@ -410,7 +410,7 @@ func showComments( for _, constraintComment := range tc.constraints { f.WriteString(";\n") - c, err := table.FindConstraintWithID(descpb.ConstraintID(constraintComment.subID)) + c, err := catalog.MustFindConstraintByID(table, descpb.ConstraintID(constraintComment.subID)) if err != nil { return err } From 3c3470ad3378fa731ae8cfbf13177e8d398dc4fe Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Wed, 11 Jan 2023 10:29:00 -0500 Subject: [PATCH 7/9] catalog: replace FindFamilyByID method with functions This commit refactors this method as a function. Informs #91924. Release note: None --- .../changefeedccl/cdceval/expr_eval_test.go | 2 +- pkg/ccl/changefeedccl/cdceval/validation.go | 2 +- pkg/ccl/changefeedccl/cdcevent/event.go | 2 +- pkg/ccl/changefeedccl/cdcevent/event_test.go | 2 +- .../cdcevent/rowfetcher_cache.go | 2 +- pkg/sql/catalog/descriptor.go | 2 -- pkg/sql/catalog/table_elements.go | 27 ++++++++++++++++++- pkg/sql/catalog/tabledesc/structured.go | 11 -------- pkg/sql/distsql_plan_changefeed.go | 2 +- 9 files changed, 32 insertions(+), 20 deletions(-) diff --git a/pkg/ccl/changefeedccl/cdceval/expr_eval_test.go b/pkg/ccl/changefeedccl/cdceval/expr_eval_test.go index cc9ff4c7453f..b6a5dba30ff0 100644 --- a/pkg/ccl/changefeedccl/cdceval/expr_eval_test.go +++ b/pkg/ccl/changefeedccl/cdceval/expr_eval_test.go @@ -727,7 +727,7 @@ func randEncDatumPrimaryFamily( t.Helper() rng, _ := randutil.NewTestRand() - family, err := desc.FindFamilyByID(0) + family, err := catalog.MustFindFamilyByID(desc, 0 /* id */) require.NoError(t, err) for _, colID := range family.ColumnIDs { col, err := desc.FindColumnWithID(colID) diff --git a/pkg/ccl/changefeedccl/cdceval/validation.go b/pkg/ccl/changefeedccl/cdceval/validation.go index 36ea0d20fd07..6a66fadb5dff 100644 --- a/pkg/ccl/changefeedccl/cdceval/validation.go +++ b/pkg/ccl/changefeedccl/cdceval/validation.go @@ -228,7 +228,7 @@ func getTargetFamilyDescriptor( ) (*descpb.ColumnFamilyDescriptor, error) { switch target.Type { case jobspb.ChangefeedTargetSpecification_PRIMARY_FAMILY_ONLY: - return desc.FindFamilyByID(0) + return catalog.MustFindFamilyByID(desc, 0 /* id */) case jobspb.ChangefeedTargetSpecification_COLUMN_FAMILY: var fd *descpb.ColumnFamilyDescriptor for _, family := range desc.GetFamilies() { diff --git a/pkg/ccl/changefeedccl/cdcevent/event.go b/pkg/ccl/changefeedccl/cdcevent/event.go index a0a4c05523c7..b0eabe2f16a8 100644 --- a/pkg/ccl/changefeedccl/cdcevent/event.go +++ b/pkg/ccl/changefeedccl/cdcevent/event.go @@ -611,7 +611,7 @@ func (it iter) Col(fn ColumnFn) error { func TestingMakeEventRow( desc catalog.TableDescriptor, familyID descpb.FamilyID, encRow rowenc.EncDatumRow, deleted bool, ) Row { - family, err := desc.FindFamilyByID(familyID) + family, err := catalog.MustFindFamilyByID(desc, familyID) if err != nil { panic(err) // primary column family always exists. } diff --git a/pkg/ccl/changefeedccl/cdcevent/event_test.go b/pkg/ccl/changefeedccl/cdcevent/event_test.go index a4122b3a858c..dabc97f5feaf 100644 --- a/pkg/ccl/changefeedccl/cdcevent/event_test.go +++ b/pkg/ccl/changefeedccl/cdcevent/event_test.go @@ -615,7 +615,7 @@ func mustGetFamily( t *testing.T, desc catalog.TableDescriptor, familyID descpb.FamilyID, ) *descpb.ColumnFamilyDescriptor { t.Helper() - f, err := desc.FindFamilyByID(familyID) + f, err := catalog.MustFindFamilyByID(desc, familyID) require.NoError(t, err) return f } diff --git a/pkg/ccl/changefeedccl/cdcevent/rowfetcher_cache.go b/pkg/ccl/changefeedccl/cdcevent/rowfetcher_cache.go index aa7be3002375..4cfd4f87c370 100644 --- a/pkg/ccl/changefeedccl/cdcevent/rowfetcher_cache.go +++ b/pkg/ccl/changefeedccl/cdcevent/rowfetcher_cache.go @@ -200,7 +200,7 @@ func (c *rowFetcherCache) RowFetcherForColumnFamily( } } - familyDesc, err := tableDesc.FindFamilyByID(family) + familyDesc, err := catalog.MustFindFamilyByID(tableDesc, family) if err != nil { return nil, nil, err } diff --git a/pkg/sql/catalog/descriptor.go b/pkg/sql/catalog/descriptor.go index 44742d7ee2be..100319b156ae 100644 --- a/pkg/sql/catalog/descriptor.go +++ b/pkg/sql/catalog/descriptor.go @@ -551,8 +551,6 @@ type TableDescriptor interface { GetFamilies() []descpb.ColumnFamilyDescriptor // NumFamilies returns the number of column families in the descriptor. NumFamilies() int - // FindFamilyByID finds the family with specified ID. - FindFamilyByID(id descpb.FamilyID) (*descpb.ColumnFamilyDescriptor, error) // ForeachFamily calls f for every column family key in desc until an // error is returned. ForeachFamily(f func(family *descpb.ColumnFamilyDescriptor) error) error diff --git a/pkg/sql/catalog/table_elements.go b/pkg/sql/catalog/table_elements.go index ae8c70c341a8..3d41f8915d42 100644 --- a/pkg/sql/catalog/table_elements.go +++ b/pkg/sql/catalog/table_elements.go @@ -932,7 +932,7 @@ func UserDefinedTypeColsHaveSameVersion(desc TableDescriptor, otherDesc TableDes func UserDefinedTypeColsInFamilyHaveSameVersion( desc TableDescriptor, otherDesc TableDescriptor, familyID descpb.FamilyID, ) (bool, error) { - family, err := desc.FindFamilyByID(familyID) + family, err := MustFindFamilyByID(desc, familyID) if err != nil { return false, err } @@ -1156,3 +1156,28 @@ func MustFindConstraintWithName(tbl TableDescriptor, name string) (Constraint, e // silence the linter var _ = MustFindConstraintWithName + +// FindFamilyByID traverses the family descriptors on the table descriptor +// and returns the first column family with the desired ID, or nil if none was +// found. +func FindFamilyByID(tbl TableDescriptor, id descpb.FamilyID) (ret *descpb.ColumnFamilyDescriptor) { + _ = tbl.ForeachFamily(func(family *descpb.ColumnFamilyDescriptor) error { + if family.ID == id { + ret = family + return iterutil.StopIteration() + } + return nil + }) + return ret +} + +// MustFindFamilyByID is like FindFamilyByID but returns an error if no column +// family was found. +func MustFindFamilyByID( + tbl TableDescriptor, id descpb.FamilyID, +) (*descpb.ColumnFamilyDescriptor, error) { + if f := FindFamilyByID(tbl, id); f != nil { + return f, nil + } + return nil, fmt.Errorf("family-id \"%d\" does not exist", id) +} diff --git a/pkg/sql/catalog/tabledesc/structured.go b/pkg/sql/catalog/tabledesc/structured.go index 7201a8d66547..3c55331854a4 100644 --- a/pkg/sql/catalog/tabledesc/structured.go +++ b/pkg/sql/catalog/tabledesc/structured.go @@ -1156,17 +1156,6 @@ func (desc *Mutable) FindActiveOrNewColumnByName(name tree.Name) (catalog.Column return nil, colinfo.NewUndefinedColumnError(string(name)) } -// FindFamilyByID implements the TableDescriptor interface. -func (desc *wrapper) FindFamilyByID(id descpb.FamilyID) (*descpb.ColumnFamilyDescriptor, error) { - for i := range desc.Families { - family := &desc.Families[i] - if family.ID == id { - return family, nil - } - } - return nil, fmt.Errorf("family-id \"%d\" does not exist", id) -} - // DropConstraint drops a constraint, either by removing it from the table // descriptor or by queuing a mutation for a schema change. func (desc *Mutable) DropConstraint( diff --git a/pkg/sql/distsql_plan_changefeed.go b/pkg/sql/distsql_plan_changefeed.go index 88c549fe99b0..0652d25312d3 100644 --- a/pkg/sql/distsql_plan_changefeed.go +++ b/pkg/sql/distsql_plan_changefeed.go @@ -439,7 +439,7 @@ func newFamilyTableDescriptor( ) (catalog.TableDescriptor, error) { // Build the set of columns in the family, along with the primary // key columns. - fam, err := original.FindFamilyByID(familyID) + fam, err := catalog.MustFindFamilyByID(original, familyID) if err != nil { return nil, err } From a227cde88f70433049bb7e3364578e8f8042046e Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Wed, 11 Jan 2023 13:17:46 -0500 Subject: [PATCH 8/9] catalog: replace FindColumnWith* methods with functions This commit refactors these methods as functions. It also moves similar functions out of tabledesc into catalog without changing them. Informs #91924. Release note: None --- pkg/ccl/changefeedccl/cdceval/cdc_prev.go | 3 +- pkg/ccl/changefeedccl/cdceval/expr_eval.go | 2 +- .../changefeedccl/cdceval/expr_eval_test.go | 2 +- pkg/ccl/changefeedccl/cdceval/validation.go | 2 +- pkg/ccl/changefeedccl/cdcevent/event_test.go | 2 +- .../schemafeed/table_event_filter.go | 2 +- .../replicationtestutils/encoding.go | 2 +- pkg/jobs/BUILD.bazel | 1 + pkg/jobs/registry_test.go | 4 +- pkg/sql/add_column.go | 7 +- pkg/sql/alter_column_type.go | 3 +- pkg/sql/alter_primary_key.go | 10 +- pkg/sql/alter_table.go | 14 +- pkg/sql/alter_table_locality.go | 2 +- pkg/sql/backfill.go | 6 +- pkg/sql/backfill/backfill.go | 4 +- pkg/sql/catalog/catformat/index.go | 4 +- pkg/sql/catalog/colinfo/column_resolver.go | 2 +- pkg/sql/catalog/descriptor.go | 15 -- pkg/sql/catalog/funcdesc/func_desc.go | 3 +- pkg/sql/catalog/schemaexpr/column.go | 4 +- pkg/sql/catalog/schemaexpr/computed_column.go | 2 +- pkg/sql/catalog/schemaexpr/expr.go | 7 +- pkg/sql/catalog/schemaexpr/partial_index.go | 2 +- pkg/sql/catalog/table_elements.go | 152 ++++++++++++++++++ pkg/sql/catalog/tabledesc/structured.go | 20 ++- pkg/sql/catalog/tabledesc/table.go | 91 +---------- pkg/sql/catalog/tabledesc/table_desc.go | 46 +----- pkg/sql/catalog/tabledesc/ttl.go | 2 +- pkg/sql/catalog/tabledesc/validate.go | 12 +- pkg/sql/check.go | 6 +- pkg/sql/comment_on_column.go | 2 +- pkg/sql/crdb_internal.go | 3 +- pkg/sql/create_index.go | 27 ++-- pkg/sql/create_stats.go | 11 +- pkg/sql/create_table.go | 18 ++- pkg/sql/descriptor_mutation_test.go | 3 +- pkg/sql/distsql_physical_planner.go | 2 +- pkg/sql/distsql_plan_changefeed.go | 21 ++- pkg/sql/distsql_plan_changefeed_test.go | 4 +- pkg/sql/distsql_plan_stats.go | 2 +- pkg/sql/drop_index.go | 4 +- pkg/sql/drop_test.go | 4 +- pkg/sql/evalcatalog/encode_table_index_key.go | 6 +- pkg/sql/evalcatalog/pg_updatable.go | 13 +- pkg/sql/importer/import_planning.go | 2 +- pkg/sql/information_schema.go | 3 +- pkg/sql/opt_catalog.go | 2 +- pkg/sql/pg_catalog.go | 6 +- pkg/sql/randgen/schema.go | 4 +- pkg/sql/rename_column.go | 2 +- pkg/sql/resolver.go | 2 +- pkg/sql/row/deleter.go | 2 +- pkg/sql/row/errors.go | 2 +- pkg/sql/rowenc/index_encoding.go | 2 +- pkg/sql/rowenc/index_fetch.go | 2 +- pkg/sql/rowenc/index_fetch_test.go | 3 +- pkg/sql/rowenc/partition.go | 2 +- pkg/sql/rowexec/inverted_joiner_test.go | 3 +- pkg/sql/rowexec/utils_test.go | 3 +- pkg/sql/scan.go | 2 +- pkg/sql/scatter.go | 3 +- pkg/sql/schema_changer.go | 2 +- .../schemachanger/scbuild/builder_state.go | 4 +- .../scexec/exec_backfill_test.go | 3 +- .../scexec/scmutationexec/column.go | 10 +- .../scexec/scmutationexec/eventlog.go | 2 +- .../scexec/scmutationexec/index.go | 4 +- .../scexec/scmutationexec/references.go | 4 +- pkg/sql/scrub_fk.go | 5 +- pkg/sql/scrub_unique_constraint.go | 2 +- pkg/sql/sequence.go | 4 +- pkg/sql/show_create_clauses.go | 4 +- pkg/sql/show_fingerprints.go | 6 +- pkg/sql/show_stats.go | 19 +-- pkg/sql/tests/hash_sharded_test.go | 3 +- pkg/sql/ttl/ttljob/BUILD.bazel | 2 + pkg/sql/ttl/ttljob/ttljob_processor.go | 3 +- pkg/sql/ttl/ttljob/ttljob_test.go | 3 +- pkg/sql/type_change.go | 2 +- pkg/upgrade/upgrades/schema_changes.go | 31 +--- 81 files changed, 341 insertions(+), 371 deletions(-) diff --git a/pkg/ccl/changefeedccl/cdceval/cdc_prev.go b/pkg/ccl/changefeedccl/cdceval/cdc_prev.go index 8e22f872e78c..ef7391114478 100644 --- a/pkg/ccl/changefeedccl/cdceval/cdc_prev.go +++ b/pkg/ccl/changefeedccl/cdceval/cdc_prev.go @@ -32,8 +32,7 @@ var _ catalog.Column = (*prevCol)(nil) func newPrevColumnForDesc(desc *cdcevent.EventDescriptor) (catalog.Column, error) { colExists := func(n tree.Name) bool { - _, err := desc.TableDescriptor().FindColumnWithName(n) - return err == nil + return catalog.FindColumnByTreeName(desc.TableDescriptor(), n) != nil } prevColName := tree.Name("cdc_prev") diff --git a/pkg/ccl/changefeedccl/cdceval/expr_eval.go b/pkg/ccl/changefeedccl/cdceval/expr_eval.go index 2cf304c463de..71f3b90a765f 100644 --- a/pkg/ccl/changefeedccl/cdceval/expr_eval.go +++ b/pkg/ccl/changefeedccl/cdceval/expr_eval.go @@ -336,7 +336,7 @@ func inputSpecForEventDescriptor( inputTypes := make([]*types.T, 0, numCols) var inputCols catalog.TableColMap for i, c := range ed.ResultColumns() { - col, err := ed.TableDescriptor().FindColumnWithName(tree.Name(c.Name)) + col, err := catalog.MustFindColumnByName(ed.TableDescriptor(), c.Name) if err != nil { return inputTypes, inputCols, err } diff --git a/pkg/ccl/changefeedccl/cdceval/expr_eval_test.go b/pkg/ccl/changefeedccl/cdceval/expr_eval_test.go index b6a5dba30ff0..989ecc3793e1 100644 --- a/pkg/ccl/changefeedccl/cdceval/expr_eval_test.go +++ b/pkg/ccl/changefeedccl/cdceval/expr_eval_test.go @@ -730,7 +730,7 @@ func randEncDatumPrimaryFamily( family, err := catalog.MustFindFamilyByID(desc, 0 /* id */) require.NoError(t, err) for _, colID := range family.ColumnIDs { - col, err := desc.FindColumnWithID(colID) + col, err := catalog.MustFindColumnByID(desc, colID) require.NoError(t, err) row = append(row, rowenc.EncDatum{Datum: randgen.RandDatum(rng, col.GetType(), col.IsNullable())}) } diff --git a/pkg/ccl/changefeedccl/cdceval/validation.go b/pkg/ccl/changefeedccl/cdceval/validation.go index 6a66fadb5dff..c77afddfbdce 100644 --- a/pkg/ccl/changefeedccl/cdceval/validation.go +++ b/pkg/ccl/changefeedccl/cdceval/validation.go @@ -366,7 +366,7 @@ func (c *checkColumnsVisitor) VisitCols(expr tree.Expr) (bool, tree.Expr) { return c.VisitCols(vn) case *tree.ColumnItem: - col, err := c.desc.FindColumnWithName(e.ColumnName) + col, err := catalog.MustFindColumnByTreeName(c.desc, e.ColumnName) if err != nil { c.err = err return false, expr diff --git a/pkg/ccl/changefeedccl/cdcevent/event_test.go b/pkg/ccl/changefeedccl/cdcevent/event_test.go index dabc97f5feaf..4d3c1446b8af 100644 --- a/pkg/ccl/changefeedccl/cdcevent/event_test.go +++ b/pkg/ccl/changefeedccl/cdcevent/event_test.go @@ -660,7 +660,7 @@ func expectResultColumns( } for _, colName := range colNames { - col, err := desc.FindColumnWithName(tree.Name(colName)) + col, err := catalog.MustFindColumnByName(desc, colName) require.NoError(t, err) res = append(res, ResultColumn{ ResultColumn: colinfo.ResultColumn{ diff --git a/pkg/ccl/changefeedccl/schemafeed/table_event_filter.go b/pkg/ccl/changefeedccl/schemafeed/table_event_filter.go index 7a8c1cabe47f..bdb5fa87f7e2 100644 --- a/pkg/ccl/changefeedccl/schemafeed/table_event_filter.go +++ b/pkg/ccl/changefeedccl/schemafeed/table_event_filter.go @@ -375,7 +375,7 @@ func hasNewPrimaryIndexWithNoVisibleColumnChanges( for i, n := 0, idx.NumPrimaryStoredColumns(); i < n; i++ { colID := idx.GetStoredColumnID(i) - col, _ := tab.FindColumnWithID(colID) + col := catalog.FindColumnByID(tab, colID) // If specific columns are targeted, then only consider the column if it is targeted. if col.Public() && (!hasSpecificColumnTargets || targetedCols.Contains(int(col.GetID()))) { diff --git a/pkg/ccl/streamingccl/replicationtestutils/encoding.go b/pkg/ccl/streamingccl/replicationtestutils/encoding.go index dbc451f4b36c..c4813593c3fb 100644 --- a/pkg/ccl/streamingccl/replicationtestutils/encoding.go +++ b/pkg/ccl/streamingccl/replicationtestutils/encoding.go @@ -33,7 +33,7 @@ func EncodeKV( var colMap catalog.TableColMap for i, val := range pkeyVals { datums = append(datums, nativeToDatum(t, val)) - col, err := descr.FindColumnWithID(descpb.ColumnID(i + 1)) + col, err := catalog.MustFindColumnByID(descr, descpb.ColumnID(i+1)) require.NoError(t, err) colMap.Set(col.GetID(), col.Ordinal()) } diff --git a/pkg/jobs/BUILD.bazel b/pkg/jobs/BUILD.bazel index 9217d9e939df..c832dba27a5e 100644 --- a/pkg/jobs/BUILD.bazel +++ b/pkg/jobs/BUILD.bazel @@ -117,6 +117,7 @@ go_test( "//pkg/settings/cluster", "//pkg/spanconfig", "//pkg/sql", + "//pkg/sql/catalog", "//pkg/sql/catalog/catalogkeys", "//pkg/sql/catalog/colinfo", "//pkg/sql/catalog/descpb", diff --git a/pkg/jobs/registry_test.go b/pkg/jobs/registry_test.go index 0d5bd4f436a2..664752b023fa 100644 --- a/pkg/jobs/registry_test.go +++ b/pkg/jobs/registry_test.go @@ -28,12 +28,12 @@ import ( "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/spanconfig" + "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/desctestutils" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/isql" - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/skip" @@ -59,7 +59,7 @@ func writeColumnMutation( column string, m descpb.DescriptorMutation, ) { - col, err := tableDesc.FindColumnWithName(tree.Name(column)) + col, err := catalog.MustFindColumnByName(tableDesc, column) if err != nil { t.Fatal(err) } diff --git a/pkg/sql/add_column.go b/pkg/sql/add_column.go index 19334247f3df..a6a9a9dd0e9b 100644 --- a/pkg/sql/add_column.go +++ b/pkg/sql/add_column.go @@ -207,10 +207,9 @@ func (p *planner) addColumnImpl( } if col.Virtual && !col.Nullable { - colName := tree.Name(col.Name) - newCol, err := n.tableDesc.FindColumnWithName(colName) + newCol, err := catalog.MustFindColumnByName(n.tableDesc, col.Name) if err != nil { - return errors.NewAssertionErrorWithWrappedErrf(err, "failed to find newly added column %v", colName) + return errors.NewAssertionErrorWithWrappedErrf(err, "failed to find newly added column %v", col.Name) } if err := addNotNullConstraintMutationForCol(n.tableDesc, newCol); err != nil { return err @@ -223,7 +222,7 @@ func (p *planner) addColumnImpl( func checkColumnDoesNotExist( tableDesc catalog.TableDescriptor, name tree.Name, ) (isPublic bool, err error) { - col, _ := tableDesc.FindColumnWithName(name) + col := catalog.FindColumnByTreeName(tableDesc, name) if col == nil { return false, nil } diff --git a/pkg/sql/alter_column_type.go b/pkg/sql/alter_column_type.go index 50cee7b3c1c5..25b8864ecffa 100644 --- a/pkg/sql/alter_column_type.go +++ b/pkg/sql/alter_column_type.go @@ -260,8 +260,7 @@ func alterColumnTypeGeneral( } nameExists := func(name string) bool { - _, err := tableDesc.FindColumnWithName(tree.Name(name)) - return err == nil + return catalog.FindColumnByName(tableDesc, name) != nil } shadowColName := tabledesc.GenerateUniqueName(col.GetName(), nameExists) diff --git a/pkg/sql/alter_primary_key.go b/pkg/sql/alter_primary_key.go index d77f9e66d57b..d1184a4bd094 100644 --- a/pkg/sql/alter_primary_key.go +++ b/pkg/sql/alter_primary_key.go @@ -133,7 +133,7 @@ func (p *planner) AlterPrimaryKey( "use columns instead", ) } - col, err := tableDesc.FindColumnWithName(elem.Column) + col, err := catalog.MustFindColumnByTreeName(tableDesc, elem.Column) if err != nil { return err } @@ -432,7 +432,7 @@ func (p *planner) AlterPrimaryKey( if idx.IsUnique() { for i := 0; i < idx.NumKeyColumns(); i++ { colID := idx.GetKeyColumnID(i) - col, err := tableDesc.FindColumnWithID(colID) + col, err := catalog.MustFindColumnByID(tableDesc, colID) if err != nil { return false, err } @@ -450,7 +450,7 @@ func (p *planner) AlterPrimaryKey( newPrimaryKeyColIDs := catalog.MakeTableColSet(newPrimaryIndexDesc.KeyColumnIDs...) for i := 0; i < idx.NumKeySuffixColumns(); i++ { colID := idx.GetKeySuffixColumnID(i) - col, err := tableDesc.FindColumnWithID(colID) + col, err := catalog.MustFindColumnByID(tableDesc, colID) if err != nil { return false, err } @@ -628,7 +628,7 @@ func (p *planner) shouldCreateIndexes( // Validate the columns on the indexes for idx, elem := range alterPKNode.Columns { - col, err := desc.FindColumnWithName(elem.Column) + col, err := catalog.MustFindColumnByTreeName(desc, elem.Column) if err != nil { return true, err } @@ -792,7 +792,7 @@ func setKeySuffixColumnIDsFromPrimary( // toAdd.KeySuffixColumnIDs = append(toAdd.KeySuffixColumnIDs, colID) // However, this functionality is not supported by the execution engine, // so prevent it by returning an error. - col, err := table.FindColumnWithID(colID) + col, err := catalog.MustFindColumnByID(table, colID) if err != nil { return err } diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index 0ee414fd68de..6044c5bbc8b5 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -276,7 +276,7 @@ func (n *alterTableNode) startExec(params runParams) error { "cannot create a unique constraint on an expression, use UNIQUE INDEX instead", ) } - _, err := n.tableDesc.FindColumnWithName(column.Column) + _, err := catalog.MustFindColumnByTreeName(n.tableDesc, column.Column) if err != nil { return err } @@ -571,7 +571,7 @@ func (n *alterTableNode) startExec(params runParams) error { case tree.ColumnMutationCmd: // Column mutations tableDesc := n.tableDesc - col, err := tableDesc.FindColumnWithName(t.GetColumn()) + col, err := catalog.MustFindColumnByTreeName(tableDesc, t.GetColumn()) if err != nil { return err } @@ -1249,8 +1249,8 @@ StatsLoop: columnIDs := tree.NewDArray(types.Int) for _, colName := range s.Columns { - col, err := desc.FindColumnWithName(tree.Name(colName)) - if err != nil { + col := catalog.FindColumnByName(desc, colName) + if col == nil { params.p.BufferClientNotice( params.ctx, pgnotice.Newf("column %q does not exist", colName), @@ -1538,7 +1538,7 @@ func dropColumnImpl( } } - colToDrop, err := tableDesc.FindColumnWithName(t.Column) + colToDrop, err := catalog.MustFindColumnByTreeName(tableDesc, t.Column) if err != nil { if t.IfExists { // Noop. @@ -1784,7 +1784,7 @@ func handleTTLStorageParamChange( // Update default expression on automated column if required. if before.HasDurationExpr() && after.HasDurationExpr() && before.DurationExpr != after.DurationExpr { - col, err := tableDesc.FindColumnWithName(colinfo.TTLDefaultExpirationColumnName) + col, err := catalog.MustFindColumnByName(tableDesc, colinfo.TTLDefaultExpirationColumnName) if err != nil { return false, err } @@ -1826,7 +1826,7 @@ func handleTTLStorageParamChange( // Adding a TTL requires adding the automatic column and deferring the TTL // addition to after the column is successfully added. addTTLMutation = true - if _, err := tableDesc.FindColumnWithName(colinfo.TTLDefaultExpirationColumnName); err == nil { + if catalog.FindColumnByName(tableDesc, colinfo.TTLDefaultExpirationColumnName) != nil { return false, pgerror.Newf( pgcode.InvalidTableDefinition, "cannot add TTL to table with the %s column already defined", diff --git a/pkg/sql/alter_table_locality.go b/pkg/sql/alter_table_locality.go index 8a74370837d8..a4c0820f575d 100644 --- a/pkg/sql/alter_table_locality.go +++ b/pkg/sql/alter_table_locality.go @@ -234,7 +234,7 @@ func (n *alterTableSetLocalityNode) alterTableLocalityToRegionalByRow( mayNeedImplicitCRDBRegionCol = true } - partCol, err := n.tableDesc.FindColumnWithName(partColName) + partCol, err := catalog.MustFindColumnByTreeName(n.tableDesc, partColName) createDefaultRegionCol := mayNeedImplicitCRDBRegionCol && sqlerrors.IsUndefinedColumnError(err) if err != nil && !createDefaultRegionCol { return err diff --git a/pkg/sql/backfill.go b/pkg/sql/backfill.go index 3595058ccdf0..e62264454de4 100644 --- a/pkg/sql/backfill.go +++ b/pkg/sql/backfill.go @@ -413,7 +413,7 @@ func shouldSkipConstraintValidation( return false, nil } - checkCol, err := tableDesc.FindColumnWithID(check.GetReferencedColumnID(0)) + checkCol, err := catalog.MustFindColumnByID(tableDesc, check.GetReferencedColumnID(0)) if err != nil { return false, err } @@ -644,7 +644,7 @@ func (sc *SchemaChanger) addConstraints( // referenced table. It's possible for the unique index found during // planning to have been dropped in the meantime, since only the // presence of the backreference prevents it. - _, err = tabledesc.FindFKReferencedUniqueConstraint(backrefTable, fk) + _, err = catalog.FindFKReferencedUniqueConstraint(backrefTable, fk) if err != nil { return err } @@ -1723,7 +1723,7 @@ func countExpectedRowsForInvertedIndex( } colID := idx.InvertedColumnID() - col, err := desc.FindColumnWithID(colID) + col, err := catalog.MustFindColumnByID(desc, colID) if err != nil { return 0, err } diff --git a/pkg/sql/backfill/backfill.go b/pkg/sql/backfill/backfill.go index 4e1b0da02b2b..e005dd423322 100644 --- a/pkg/sql/backfill/backfill.go +++ b/pkg/sql/backfill/backfill.go @@ -604,9 +604,9 @@ func constructExprs( if addedColSet.Contains(colID) { continue } - col, err := desc.FindColumnWithID(colID) + col, err := catalog.MustFindColumnByID(desc, colID) if err != nil { - return errors.AssertionFailedf("column %d does not exist", colID) + return errors.HandleAsAssertionFailure(err) } if col.IsVirtual() { continue diff --git a/pkg/sql/catalog/catformat/index.go b/pkg/sql/catalog/catformat/index.go index ce1d04f66868..41cb203d72dd 100644 --- a/pkg/sql/catalog/catformat/index.go +++ b/pkg/sql/catalog/catformat/index.go @@ -201,7 +201,7 @@ func FormatIndexElements( startIdx := index.ExplicitColumnStartIdx() for i, n := startIdx, len(index.KeyColumnIDs); i < n; i++ { - col, err := table.FindColumnWithID(index.KeyColumnIDs[i]) + col, err := catalog.MustFindColumnByID(table, index.KeyColumnIDs[i]) if err != nil { return err } @@ -279,7 +279,7 @@ func formatStorageConfigs( } if index.GeoConfig.S2Geometry != nil { - col, err := table.FindColumnWithID(index.InvertedColumnID()) + col, err := catalog.MustFindColumnByID(table, index.InvertedColumnID()) if err != nil { return errors.Wrapf(err, "expected column %q to exist in table", index.InvertedColumnName()) } diff --git a/pkg/sql/catalog/colinfo/column_resolver.go b/pkg/sql/catalog/colinfo/column_resolver.go index 0c59c54c2622..acfdb995f493 100644 --- a/pkg/sql/catalog/colinfo/column_resolver.go +++ b/pkg/sql/catalog/colinfo/column_resolver.go @@ -43,7 +43,7 @@ func ProcessTargetColumns( var colIDSet catalog.TableColSet cols := make([]catalog.Column, len(nameList)) for i, colName := range nameList { - col, err := tableDesc.FindColumnWithName(colName) + col, err := catalog.MustFindColumnByTreeName(tableDesc, colName) if err != nil { return nil, err } diff --git a/pkg/sql/catalog/descriptor.go b/pkg/sql/catalog/descriptor.go index 100319b156ae..2d368cdae5a9 100644 --- a/pkg/sql/catalog/descriptor.go +++ b/pkg/sql/catalog/descriptor.go @@ -521,21 +521,6 @@ type TableDescriptor interface { // suffix columns, suitable for populating a fetchpb.IndexFetchSpec. IndexFetchSpecKeyAndSuffixColumns(idx Index) []fetchpb.IndexFetchSpec_KeyColumn - // FindColumnWithID returns the first column found whose ID matches the - // provided target ID, in the canonical order. - // If no column is found then an error is also returned. - FindColumnWithID(id descpb.ColumnID) (Column, error) - - // FindColumnWithPGAttributeNum returns the first column found whose - // PGAttributeNum (if set, otherwise ID) matches the provider id. - // Error is returned if no column is found. - FindColumnWithPGAttributeNum(id descpb.PGAttributeNum) (Column, error) - - // FindColumnWithName returns the first column found whose name matches the - // provided target name, in the canonical order. - // If no column is found then an error is also returned. - FindColumnWithName(name tree.Name) (Column, error) - // GetNextColumnID returns the next unused column ID for this table. Column // IDs are unique per table, but not unique globally. GetNextColumnID() descpb.ColumnID diff --git a/pkg/sql/catalog/funcdesc/func_desc.go b/pkg/sql/catalog/funcdesc/func_desc.go index 83a03e968a58..4edeb1fc850c 100644 --- a/pkg/sql/catalog/funcdesc/func_desc.go +++ b/pkg/sql/catalog/funcdesc/func_desc.go @@ -313,8 +313,7 @@ func (desc *immutable) validateInboundTableRef( } for _, colID := range by.ColumnIDs { - _, err := backRefTbl.FindColumnWithID(colID) - if err != nil { + if catalog.FindColumnByID(backRefTbl, colID) == nil { return errors.AssertionFailedf("depended-on-by relation %q (%d) does not have a column with ID %d", backRefTbl.GetName(), by.ID, colID) } diff --git a/pkg/sql/catalog/schemaexpr/column.go b/pkg/sql/catalog/schemaexpr/column.go index 44eb16d2a267..4c0971824cbb 100644 --- a/pkg/sql/catalog/schemaexpr/column.go +++ b/pkg/sql/catalog/schemaexpr/column.go @@ -202,7 +202,7 @@ func iterColDescriptors( return true, expr, nil } - col, err := desc.FindColumnWithName(c.ColumnName) + col, err := catalog.MustFindColumnByTreeName(desc, c.ColumnName) if err != nil || col.Dropped() { return false, nil, pgerror.Newf(pgcode.UndefinedColumn, "column %q does not exist, referenced in %q", c.ColumnName, rootExpr.String()) @@ -313,7 +313,7 @@ func replaceColumnVars( tbl catalog.TableDescriptor, rootExpr tree.Expr, ) (tree.Expr, catalog.TableColSet, error) { lookupFn := func(columnName tree.Name) (exists bool, accessible bool, id catid.ColumnID, typ *types.T) { - col, err := tbl.FindColumnWithName(columnName) + col, err := catalog.MustFindColumnByTreeName(tbl, columnName) if err != nil || col.Dropped() { return false, false, 0, nil } diff --git a/pkg/sql/catalog/schemaexpr/computed_column.go b/pkg/sql/catalog/schemaexpr/computed_column.go index e8e521d159ed..141ba52a1dad 100644 --- a/pkg/sql/catalog/schemaexpr/computed_column.go +++ b/pkg/sql/catalog/schemaexpr/computed_column.go @@ -120,7 +120,7 @@ func ValidateComputedColumnExpression( return } var col catalog.Column - if col, err = desc.FindColumnWithID(colID); err != nil { + if col, err = catalog.MustFindColumnByID(desc, colID); err != nil { err = errors.WithAssertionFailure(err) return } diff --git a/pkg/sql/catalog/schemaexpr/expr.go b/pkg/sql/catalog/schemaexpr/expr.go index 097890224d53..59faddc63e7f 100644 --- a/pkg/sql/catalog/schemaexpr/expr.go +++ b/pkg/sql/catalog/schemaexpr/expr.go @@ -140,7 +140,7 @@ func DequalifyAndValidateExpr( return colinfo.ResultColumnsFromColumns(desc.GetID(), desc.NonDropColumns()) } columnLookupByNameFn := func(columnName tree.Name) (exists bool, accessible bool, id catid.ColumnID, typ *types.T) { - col, err := desc.FindColumnWithName(columnName) + col, err := catalog.MustFindColumnByTreeName(desc, columnName) if err != nil || col.Dropped() { return false, false, 0, nil } @@ -173,7 +173,7 @@ func ExtractColumnIDs( return true, expr, nil } - col, err := desc.FindColumnWithName(c.ColumnName) + col, err := catalog.MustFindColumnByTreeName(desc, c.ColumnName) if err != nil { return false, nil, err } @@ -210,8 +210,7 @@ func HasValidColumnReferences(desc catalog.TableDescriptor, rootExpr tree.Expr) return true, expr, nil } - _, err = desc.FindColumnWithName(c.ColumnName) - if err != nil { + if catalog.FindColumnByTreeName(desc, c.ColumnName) == nil { return false, expr, returnFalsePseudoError } diff --git a/pkg/sql/catalog/schemaexpr/partial_index.go b/pkg/sql/catalog/schemaexpr/partial_index.go index 9d7a253d13cd..e5c421f9a086 100644 --- a/pkg/sql/catalog/schemaexpr/partial_index.go +++ b/pkg/sql/catalog/schemaexpr/partial_index.go @@ -75,7 +75,7 @@ func validatePartialIndexExprColsArePublic( return } var col catalog.Column - col, err = desc.FindColumnWithID(colID) + col, err = catalog.MustFindColumnByID(desc, colID) if err != nil { return } diff --git a/pkg/sql/catalog/table_elements.go b/pkg/sql/catalog/table_elements.go index 3d41f8915d42..d77f7402d6eb 100644 --- a/pkg/sql/catalog/table_elements.go +++ b/pkg/sql/catalog/table_elements.go @@ -1181,3 +1181,155 @@ func MustFindFamilyByID( } return nil, fmt.Errorf("family-id \"%d\" does not exist", id) } + +// FindColumnByID traverses the slice returned by the AllColumns +// method on the table descriptor and returns the first Column that +// matches the desired ID, or nil if none was found. +func FindColumnByID(tbl TableDescriptor, id descpb.ColumnID) Column { + for _, col := range tbl.AllColumns() { + if col.GetID() == id { + return col + } + } + return nil +} + +// MustFindColumnByID is like FindColumnByID but returns an error when +// no Column was found. +func MustFindColumnByID(tbl TableDescriptor, id descpb.ColumnID) (Column, error) { + if col := FindColumnByID(tbl, id); col != nil { + return col, nil + } + return nil, pgerror.Newf(pgcode.UndefinedColumn, "column-id \"%d\" does not exist", id) +} + +// FindColumnByName is like FindColumnByID but with names instead of +// IDs. +func FindColumnByName(tbl TableDescriptor, name string) Column { + for _, col := range tbl.AllColumns() { + if col.GetName() == name { + return col + } + } + return nil +} + +// MustFindColumnByName is like MustFindColumnByID but with names +// instead of IDs. +func MustFindColumnByName(tbl TableDescriptor, name string) (Column, error) { + if col := FindColumnByName(tbl, name); col != nil { + return col, nil + } + return nil, pgerror.Newf(pgcode.UndefinedColumn, "column %q does not exist", name) +} + +// FindColumnByTreeName is like FindColumnByID but with names instead of +// IDs. +func FindColumnByTreeName(tbl TableDescriptor, name tree.Name) Column { + for _, col := range tbl.AllColumns() { + if col.ColName() == name { + return col + } + } + return nil +} + +// MustFindColumnByTreeName is like MustFindColumnByID but with names +// instead of IDs. +func MustFindColumnByTreeName(tbl TableDescriptor, name tree.Name) (Column, error) { + if col := FindColumnByTreeName(tbl, name); col != nil { + return col, nil + } + return nil, pgerror.Newf(pgcode.UndefinedColumn, "column %q does not exist", name) +} + +// FindColumnByPGAttributeNum traverses the slice returned by the AllColumns +// method on the table descriptor and returns the first Column that +// matches the desired PGAttributeNum, or the ID if not set. +// Returns nil if none was found. +func FindColumnByPGAttributeNum(tbl TableDescriptor, attNum descpb.PGAttributeNum) Column { + for _, col := range tbl.AllColumns() { + if col.GetPGAttributeNum() == attNum { + return col + } + } + return nil +} + +// MustFindColumnByPGAttributeNum is like FindColumnByPGAttributeNum but returns +// an error when no column is found. +func MustFindColumnByPGAttributeNum( + tbl TableDescriptor, attNum descpb.PGAttributeNum, +) (Column, error) { + if col := FindColumnByPGAttributeNum(tbl, attNum); col != nil { + return col, nil + } + return nil, pgerror.Newf(pgcode.UndefinedColumn, + "column with logical order %d does not exist", attNum) +} + +// MustFindPublicColumnsByNameList is a convenience function which behaves +// exactly like MustFindPublicColumnByTreeName applied repeatedly to the +// names in the provided list, returning early at the first encountered error. +func MustFindPublicColumnsByNameList(desc TableDescriptor, names tree.NameList) ([]Column, error) { + cols := make([]Column, len(names)) + for i, name := range names { + c, err := MustFindPublicColumnByTreeName(desc, name) + if err != nil { + return nil, err + } + cols[i] = c + } + return cols, nil +} + +// MustFindPublicColumnByTreeName is a convenience function which behaves exactly +// like FindColumnByName except it ignores column mutations. +func MustFindPublicColumnByTreeName(desc TableDescriptor, name tree.Name) (Column, error) { + col, err := MustFindColumnByTreeName(desc, name) + if err != nil { + return nil, err + } + if !col.Public() { + return nil, pgerror.Newf(pgcode.UndefinedColumn, "column %q does not exist", name) + } + return col, nil +} + +// MustFindPublicColumnByID is a convenience function which behaves exactly +// like FindColumnByID except it ignores column mutations. +func MustFindPublicColumnByID(desc TableDescriptor, id descpb.ColumnID) (Column, error) { + col, err := MustFindColumnByID(desc, id) + if err != nil { + return nil, err + } + if !col.Public() { + return nil, fmt.Errorf("column-id \"%d\" does not exist", id) + } + return col, nil +} + +// FindFKReferencedUniqueConstraint finds the first index in the supplied +// referencedTable that can satisfy a foreign key of the supplied column ids. +// If no such index exists, attempts to find a unique constraint on the supplied +// column ids. If neither an index nor unique constraint is found, returns an +// error. +func FindFKReferencedUniqueConstraint( + referencedTable TableDescriptor, fk ForeignKeyConstraint, +) (UniqueConstraint, error) { + for _, uwi := range referencedTable.UniqueConstraintsWithIndex() { + if !uwi.Dropped() && uwi.IsValidReferencedUniqueConstraint(fk) { + return uwi, nil + } + } + for _, uwoi := range referencedTable.UniqueConstraintsWithoutIndex() { + if !uwoi.Dropped() && uwoi.IsValidReferencedUniqueConstraint(fk) { + return uwoi, nil + } + } + return nil, pgerror.Newf( + pgcode.ForeignKeyViolation, + "there is no unique constraint matching given keys for referenced table %s", + referencedTable.GetName(), + ) +} diff --git a/pkg/sql/catalog/tabledesc/structured.go b/pkg/sql/catalog/tabledesc/structured.go index 3c55331854a4..bad065d66e72 100644 --- a/pkg/sql/catalog/tabledesc/structured.go +++ b/pkg/sql/catalog/tabledesc/structured.go @@ -150,7 +150,7 @@ func BuildIndexName(tableDesc *Mutable, idx *descpb.IndexDescriptor) (string, er exprCount := 0 for i, n := idx.ExplicitColumnStartIdx(), len(idx.KeyColumnNames); i < n; i++ { var segmentName string - col, err := tableDesc.FindColumnWithName(tree.Name(idx.KeyColumnNames[i])) + col, err := catalog.MustFindColumnByName(tableDesc, idx.KeyColumnNames[i]) if err != nil { return "", err } @@ -496,8 +496,7 @@ func (desc *Mutable) ensurePrimaryKey() error { if len(desc.PrimaryIndex.KeyColumnNames) == 0 && desc.IsPhysicalTable() { // Ensure a Primary Key exists. nameExists := func(name string) bool { - _, err := desc.FindColumnWithName(tree.Name(name)) - return err == nil + return catalog.FindColumnByName(desc, name) != nil } s := "unique_rowid()" col := &descpb.ColumnDescriptor{ @@ -614,7 +613,7 @@ func (desc *Mutable) allocateIndexIDs(columnNames map[string]descpb.ColumnID) er // extraColumnIDs = append(extraColumnIDs, primaryColID) // However, this functionality is not supported by the execution engine, // so prevent it by returning an error. - col, err := desc.FindColumnWithID(primaryColID) + col, err := catalog.MustFindColumnByID(desc, primaryColID) if err != nil { return err } @@ -641,7 +640,7 @@ func (desc *Mutable) allocateIndexIDs(columnNames map[string]descpb.ColumnID) er // TODO(postamar): AllocateIDs should not do user input validation. // The only errors it should return should be assertion failures. for _, colName := range idx.IndexDesc().StoreColumnNames { - col, err := desc.FindColumnWithName(tree.Name(colName)) + col, err := catalog.MustFindColumnByName(desc, colName) if err != nil { return err } @@ -1315,7 +1314,7 @@ func (desc *wrapper) IsPrimaryIndexDefaultRowID() bool { if len(desc.PrimaryIndex.KeyColumnIDs) != 1 { return false } - col, err := desc.FindColumnWithID(desc.PrimaryIndex.KeyColumnIDs[0]) + col, err := catalog.MustFindColumnByID(desc, desc.PrimaryIndex.KeyColumnIDs[0]) if err != nil { // Should never be in this case. panic(err) @@ -1429,7 +1428,7 @@ func (desc *Mutable) MakeMutationComplete(m descpb.DescriptorMutation) error { break } } - col, err := desc.FindColumnWithID(t.Constraint.NotNullColumn) + col, err := catalog.MustFindColumnByID(desc, t.Constraint.NotNullColumn) if err != nil { return err } @@ -1546,11 +1545,11 @@ func (desc *Mutable) MakeMutationComplete(m descpb.DescriptorMutation) error { func (desc *Mutable) performComputedColumnSwap(swap *descpb.ComputedColumnSwap) error { // Get the old and new columns from the descriptor. - oldCol, err := desc.FindColumnWithID(swap.OldColumnId) + oldCol, err := catalog.MustFindColumnByID(desc, swap.OldColumnId) if err != nil { return err } - newCol, err := desc.FindColumnWithID(swap.NewColumnId) + newCol, err := catalog.MustFindColumnByID(desc, swap.NewColumnId) if err != nil { return err } @@ -1563,8 +1562,7 @@ func (desc *Mutable) performComputedColumnSwap(swap *descpb.ComputedColumnSwap) // Generate unique name for old column. nameExists := func(name string) bool { - _, err := desc.FindColumnWithName(tree.Name(name)) - return err == nil + return catalog.FindColumnByName(desc, name) != nil } uniqueName := GenerateUniqueName(newCol.GetName(), nameExists) diff --git a/pkg/sql/catalog/tabledesc/table.go b/pkg/sql/catalog/tabledesc/table.go index ac5c0da33308..1100b68ecb59 100644 --- a/pkg/sql/catalog/tabledesc/table.go +++ b/pkg/sql/catalog/tabledesc/table.go @@ -383,31 +383,6 @@ func (desc *wrapper) EnforcedUniqueConstraintsWithoutIndex() []catalog.UniqueWit return desc.getExistingOrNewConstraintCache().uwoisEnforced } -// FindFKReferencedUniqueConstraint finds the first index in the supplied -// referencedTable that can satisfy a foreign key of the supplied column ids. -// If no such index exists, attempts to find a unique constraint on the supplied -// column ids. If neither an index nor unique constraint is found, returns an -// error. -func FindFKReferencedUniqueConstraint( - referencedTable catalog.TableDescriptor, fk catalog.ForeignKeyConstraint, -) (catalog.UniqueConstraint, error) { - for _, uwi := range referencedTable.UniqueConstraintsWithIndex() { - if !uwi.Dropped() && uwi.IsValidReferencedUniqueConstraint(fk) { - return uwi, nil - } - } - for _, uwoi := range referencedTable.UniqueConstraintsWithoutIndex() { - if !uwoi.Dropped() && uwoi.IsValidReferencedUniqueConstraint(fk) { - return uwoi, nil - } - } - return nil, pgerror.Newf( - pgcode.ForeignKeyViolation, - "there is no unique constraint matching given keys for referenced table %s", - referencedTable.GetName(), - ) -} - // InitTableDescriptor returns a blank TableDescriptor. func InitTableDescriptor( id, parentID, parentSchemaID descpb.ID, @@ -434,70 +409,6 @@ func InitTableDescriptor( } } -// FindPublicColumnsWithNames is a convenience function which behaves exactly -// like FindPublicColumnWithName applied repeatedly to the names in the -// provided list, returning early at the first encountered error. -func FindPublicColumnsWithNames( - desc catalog.TableDescriptor, names tree.NameList, -) ([]catalog.Column, error) { - cols := make([]catalog.Column, len(names)) - for i, name := range names { - c, err := FindPublicColumnWithName(desc, name) - if err != nil { - return nil, err - } - cols[i] = c - } - return cols, nil -} - -// FindPublicColumnWithName is a convenience function which behaves exactly -// like desc.FindColumnWithName except it ignores column mutations. -func FindPublicColumnWithName( - desc catalog.TableDescriptor, name tree.Name, -) (catalog.Column, error) { - col, err := desc.FindColumnWithName(name) - if err != nil { - return nil, err - } - if !col.Public() { - return nil, colinfo.NewUndefinedColumnError(string(name)) - } - return col, nil -} - -// FindPublicColumnWithID is a convenience function which behaves exactly -// like desc.FindColumnWithID except it ignores column mutations. -func FindPublicColumnWithID( - desc catalog.TableDescriptor, id descpb.ColumnID, -) (catalog.Column, error) { - col, err := desc.FindColumnWithID(id) - if err != nil { - return nil, err - } - if !col.Public() { - return nil, fmt.Errorf("column-id \"%d\" does not exist", id) - } - return col, nil -} - -// FindInvertedColumn returns a catalog.Column matching the inverted column -// descriptor in `spec` if not nil, nil otherwise. -func FindInvertedColumn( - desc catalog.TableDescriptor, invertedColDesc *descpb.ColumnDescriptor, -) catalog.Column { - if invertedColDesc == nil { - return nil - } - found, err := desc.FindColumnWithID(invertedColDesc.ID) - if err != nil { - panic(errors.HandleAsAssertionFailure(err)) - } - invertedColumn := found.DeepCopy() - *invertedColumn.ColumnDesc() = *invertedColDesc - return invertedColumn -} - // PrimaryKeyString returns the pretty-printed primary key declaration for a // table descriptor. func PrimaryKeyString(desc catalog.TableDescriptor) string { @@ -674,7 +585,7 @@ func RenameColumnInTable( // Rename any shard columns which need to be renamed because their name was // based on this column. for oldShardColName, newShardColName := range shardColumnsToRename { - shardCol, err := tableDesc.FindColumnWithName(oldShardColName) + shardCol, err := catalog.MustFindColumnByTreeName(tableDesc, oldShardColName) if err != nil { return err } diff --git a/pkg/sql/catalog/tabledesc/table_desc.go b/pkg/sql/catalog/tabledesc/table_desc.go index d9005b01304d..4240890aa6e6 100644 --- a/pkg/sql/catalog/tabledesc/table_desc.go +++ b/pkg/sql/catalog/tabledesc/table_desc.go @@ -15,14 +15,10 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catenumpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/fetchpb" - "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" - "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/iterutil" @@ -505,7 +501,7 @@ func (desc *wrapper) CheckConstraintColumns(ck catalog.CheckConstraint) []catalo } ret := make([]catalog.Column, n) for i := 0; i < n; i++ { - ret[i], _ = desc.FindColumnWithID(ck.GetReferencedColumnID(i)) + ret[i] = catalog.FindColumnByID(desc, ck.GetReferencedColumnID(i)) } return ret } @@ -518,7 +514,7 @@ func (desc *wrapper) ForeignKeyReferencedColumns(fk catalog.ForeignKeyConstraint } ret := make([]catalog.Column, n) for i := 0; i < n; i++ { - ret[i], _ = desc.FindColumnWithID(fk.GetReferencedColumnID(i)) + ret[i] = catalog.FindColumnByID(desc, fk.GetReferencedColumnID(i)) } return ret } @@ -531,7 +527,7 @@ func (desc *wrapper) ForeignKeyOriginColumns(fk catalog.ForeignKeyConstraint) [] } ret := make([]catalog.Column, n) for i := 0; i < n; i++ { - ret[i], _ = desc.FindColumnWithID(fk.GetOriginColumnID(i)) + ret[i] = catalog.FindColumnByID(desc, fk.GetOriginColumnID(i)) } return ret } @@ -546,7 +542,7 @@ func (desc *wrapper) UniqueWithoutIndexColumns( } ret := make([]catalog.Column, n) for i := 0; i < n; i++ { - ret[i], _ = desc.FindColumnWithID(uwoi.GetKeyColumnID(i)) + ret[i] = catalog.FindColumnByID(desc, uwoi.GetKeyColumnID(i)) } return ret } @@ -574,40 +570,6 @@ func (desc *wrapper) getExistingOrNewIndexColumnCache(idx catalog.Index) *indexC return &c.index[idx.Ordinal()] } -// FindColumnWithID implements the TableDescriptor interface. -func (desc *wrapper) FindColumnWithID(id descpb.ColumnID) (catalog.Column, error) { - for _, col := range desc.AllColumns() { - if col.GetID() == id { - return col, nil - } - } - - return nil, pgerror.Newf(pgcode.UndefinedColumn, "column-id \"%d\" does not exist", id) -} - -// FindColumnWithPGAttributeNum implements the TableDescriptor interface. -func (desc *wrapper) FindColumnWithPGAttributeNum( - id descpb.PGAttributeNum, -) (catalog.Column, error) { - for _, col := range desc.AllColumns() { - if col.GetPGAttributeNum() == id { - return col, nil - } - } - - return nil, pgerror.Newf(pgcode.UndefinedColumn, "column with logical order %q does not exist", id) -} - -// FindColumnWithName implements the TableDescriptor interface. -func (desc *wrapper) FindColumnWithName(name tree.Name) (catalog.Column, error) { - for _, col := range desc.AllColumns() { - if col.ColName() == name { - return col, nil - } - } - return nil, colinfo.NewUndefinedColumnError(string(name)) -} - // getExistingOrNewMutationCache should be the only place where the // mutationCache field in wrapper is ever read. func (desc *wrapper) getExistingOrNewMutationCache() *mutationCache { diff --git a/pkg/sql/catalog/tabledesc/ttl.go b/pkg/sql/catalog/tabledesc/ttl.go index 7161cd9ff478..dc7828e62419 100644 --- a/pkg/sql/catalog/tabledesc/ttl.go +++ b/pkg/sql/catalog/tabledesc/ttl.go @@ -102,7 +102,7 @@ func ValidateTTLExpirationColumn(desc catalog.TableDescriptor) error { return nil } intervalExpr := desc.GetRowLevelTTL().DurationExpr - col, err := desc.FindColumnWithName(colinfo.TTLDefaultExpirationColumnName) + col, err := catalog.MustFindColumnByTreeName(desc, colinfo.TTLDefaultExpirationColumnName) if err != nil { return errors.Wrapf(err, "expected column %s", colinfo.TTLDefaultExpirationColumnName) } diff --git a/pkg/sql/catalog/tabledesc/validate.go b/pkg/sql/catalog/tabledesc/validate.go index d7f2439b3cf2..49f40c6ae33a 100644 --- a/pkg/sql/catalog/tabledesc/validate.go +++ b/pkg/sql/catalog/tabledesc/validate.go @@ -366,7 +366,7 @@ func (desc *wrapper) validateInboundTableRef( if colID == 0 { continue } - col, _ := backReferencedTable.FindColumnWithID(colID) + col := catalog.FindColumnByID(backReferencedTable, colID) if col == nil { return errors.AssertionFailedf("depended-on-by relation %q (%d) does not have a column with ID %d", backReferencedTable.GetName(), by.ID, colID) @@ -616,7 +616,7 @@ func (desc *wrapper) ValidateSelf(vea catalog.ValidationErrorAccumulator) { } } for _, colID := range ref.ColumnIDs { - if col, _ := desc.FindColumnWithID(colID); col == nil { + if catalog.FindColumnByID(desc, colID) == nil { vea.Report(errors.AssertionFailedf( "column ID %d found in depended-on-by references, no such column in this relation", colID)) @@ -875,7 +875,7 @@ func ValidateOnUpdate(desc catalog.TableDescriptor, errReportFn func(err error)) for i, n := 0, fk.NumOriginColumns(); i < n; i++ { fkCol := fk.GetOriginColumnID(i) if onUpdateCols.Contains(fkCol) { - col, err := desc.FindColumnWithID(fkCol) + col, err := catalog.MustFindColumnByID(desc, fkCol) if err != nil { errReportFn(err) } else { @@ -1323,7 +1323,7 @@ func (desc *wrapper) validateTableIndexes(columnsByID map[descpb.ColumnID]catalo if err := desc.ensureShardedIndexNotComputed(idx.IndexDesc()); err != nil { return err } - if col, _ := desc.FindColumnWithName(tree.Name(idx.GetSharded().Name)); col == nil { + if catalog.FindColumnByName(desc, idx.GetSharded().Name) == nil { return errors.Newf("index %q refers to non-existent shard column %q", idx.GetName(), idx.GetSharded().Name) } @@ -1480,7 +1480,7 @@ func (desc *wrapper) validateTableIndexes(columnsByID map[descpb.ColumnID]catalo // based on another computed column B). func (desc *wrapper) ensureShardedIndexNotComputed(index *descpb.IndexDescriptor) error { for _, colName := range index.Sharded.ColumnNames { - col, err := desc.FindColumnWithName(tree.Name(colName)) + col, err := catalog.MustFindColumnByName(desc, colName) if err != nil { return err } @@ -1543,7 +1543,7 @@ func (desc *wrapper) validatePartitioningDescriptor( if i >= idx.NumKeyColumns() { continue } - col, err := desc.FindColumnWithID(idx.GetKeyColumnID(i)) + col, err := catalog.MustFindColumnByID(desc, idx.GetKeyColumnID(i)) if err != nil { return err } diff --git a/pkg/sql/check.go b/pkg/sql/check.go index cc98f76851bd..8165e67f55c7 100644 --- a/pkg/sql/check.go +++ b/pkg/sql/check.go @@ -106,7 +106,7 @@ func matchFullUnacceptableKeyQuery( returnedCols := srcCols for i := 0; i < nCols; i++ { - col, err := srcTbl.FindColumnWithID(fk.OriginColumnIDs[i]) + col, err := catalog.MustFindColumnByID(srcTbl, fk.OriginColumnIDs[i]) if err != nil { return "", nil, err } @@ -125,7 +125,7 @@ func matchFullUnacceptableKeyQuery( } } if !alreadyPresent { - col, err := tabledesc.FindPublicColumnWithID(srcTbl, id) + col, err := catalog.MustFindPublicColumnByID(srcTbl, id) if err != nil { return "", nil, err } @@ -198,7 +198,7 @@ func nonMatchingRowQuery( } } if !found { - column, err := tabledesc.FindPublicColumnWithID(srcTbl, pkColID) + column, err := catalog.MustFindPublicColumnByID(srcTbl, pkColID) if err != nil { return "", nil, err } diff --git a/pkg/sql/comment_on_column.go b/pkg/sql/comment_on_column.go index 411e4973bb99..ca92a68e8f78 100644 --- a/pkg/sql/comment_on_column.go +++ b/pkg/sql/comment_on_column.go @@ -53,7 +53,7 @@ func (p *planner) CommentOnColumn(ctx context.Context, n *tree.CommentOnColumn) } func (n *commentOnColumnNode) startExec(params runParams) error { - col, err := n.tableDesc.FindColumnWithName(n.n.ColumnItem.ColumnName) + col, err := catalog.MustFindColumnByTreeName(n.tableDesc, n.n.ColumnItem.ColumnName) if err != nil { return err } diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index c3eef782ddce..50b3760e0654 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -54,7 +54,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr" "github.com/cockroachdb/cockroach/pkg/sql/catalog/systemschema" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc" "github.com/cockroachdb/cockroach/pkg/sql/clusterunique" "github.com/cockroachdb/cockroach/pkg/sql/idxusage" @@ -3498,7 +3497,7 @@ CREATE TABLE crdb_internal.backward_dependencies ( if err != nil { return err } - refConstraint, err := tabledesc.FindFKReferencedUniqueConstraint(refTbl, fk) + refConstraint, err := catalog.FindFKReferencedUniqueConstraint(refTbl, fk) if err != nil { return err } diff --git a/pkg/sql/create_index.go b/pkg/sql/create_index.go index c2d1239b6ce9..f7df6e09f826 100644 --- a/pkg/sql/create_index.go +++ b/pkg/sql/create_index.go @@ -31,7 +31,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" - "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/cockroach/pkg/sql/storageparam" "github.com/cockroachdb/cockroach/pkg/sql/storageparam/indexstorageparam" @@ -217,7 +216,7 @@ func makeIndexDescriptor( indexDesc.Type = descpb.IndexDescriptor_INVERTED invCol := columns[len(columns)-1] - column, err := tableDesc.FindColumnWithName(invCol.Column) + column, err := catalog.MustFindColumnByTreeName(tableDesc, invCol.Column) if err != nil { return nil, err } @@ -310,7 +309,7 @@ func checkIndexColumns( desc catalog.TableDescriptor, columns tree.IndexElemList, storing tree.NameList, inverted bool, ) error { for i, colDef := range columns { - col, err := desc.FindColumnWithName(colDef.Column) + col, err := catalog.MustFindColumnByTreeName(desc, colDef.Column) if err != nil { return errors.Wrapf(err, "finding column %d", i) } @@ -326,7 +325,7 @@ func checkIndexColumns( } } for i, colName := range storing { - col, err := desc.FindColumnWithName(colName) + col, err := catalog.MustFindColumnByTreeName(desc, colName) if err != nil { return errors.Wrapf(err, "finding store column %d", i) } @@ -424,7 +423,7 @@ func validateColumnsAreAccessible(desc *tabledesc.Mutable, columns tree.IndexEle if column.Expr != nil { continue } - foundColumn, err := desc.FindColumnWithName(column.Column) + foundColumn, err := catalog.MustFindColumnByTreeName(desc, column.Column) if err != nil { return err } @@ -446,7 +445,7 @@ func validateIndexColumnsExist(desc *tabledesc.Mutable, columns tree.IndexElemLi if column.Expr != nil { return errors.AssertionFailedf("index elem expression should have been replaced with a column") } - foundColumn, err := desc.FindColumnWithName(column.Column) + foundColumn, err := catalog.MustFindColumnByTreeName(desc, column.Column) if err != nil { return err } @@ -576,8 +575,7 @@ func replaceExpressionElemsWithVirtualCols( // Create a new virtual column and add it to the table descriptor. colName := tabledesc.GenerateUniqueName("crdb_internal_idx_expr", func(name string) bool { - _, err := desc.FindColumnWithName(tree.Name(name)) - return err == nil + return catalog.FindColumnByName(desc, name) != nil }) col := &descpb.ColumnDescriptor{ Name: colName, @@ -680,8 +678,8 @@ func maybeCreateAndAddShardCol( if err != nil { return nil, err } - existingShardCol, err := desc.FindColumnWithName(tree.Name(shardColDesc.Name)) - if err == nil && !existingShardCol.Dropped() { + existingShardCol := catalog.FindColumnByName(desc, shardColDesc.Name) + if existingShardCol != nil && !existingShardCol.Dropped() { // TODO(ajwerner): In what ways is existingShardCol allowed to differ from // the newly made shardCol? Should there be some validation of // existingShardCol? @@ -693,19 +691,14 @@ func maybeCreateAndAddShardCol( } return existingShardCol, nil } - columnIsUndefined := sqlerrors.IsUndefinedColumnError(err) - if err != nil && !columnIsUndefined { - return nil, err - } - if columnIsUndefined || existingShardCol.Dropped() { + if existingShardCol == nil || existingShardCol.Dropped() { if isNewTable { desc.AddColumn(shardColDesc) } else { desc.AddColumnMutation(shardColDesc, descpb.DescriptorMutation_ADD) } } - shardCol, err := desc.FindColumnWithName(tree.Name(shardColDesc.Name)) - return shardCol, err + return catalog.MustFindColumnByName(desc, shardColDesc.Name) } func (n *createIndexNode) startExec(params runParams) error { diff --git a/pkg/sql/create_stats.go b/pkg/sql/create_stats.go index 3b9f554d440f..323c8e480ae9 100644 --- a/pkg/sql/create_stats.go +++ b/pkg/sql/create_stats.go @@ -25,7 +25,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/isql" "github.com/cockroachdb/cockroach/pkg/sql/parser" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" @@ -270,7 +269,7 @@ func (n *createStatsNode) makeJobRecord(ctx context.Context) (*jobs.Record, erro return nil, err } } else { - columns, err := tabledesc.FindPublicColumnsWithNames(tableDesc, n.ColumnNames) + columns, err := catalog.MustFindPublicColumnsByNameList(tableDesc, n.ColumnNames) if err != nil { return nil, err } @@ -286,7 +285,7 @@ func (n *createStatsNode) makeJobRecord(ctx context.Context) (*jobs.Record, erro } columnIDs[i] = columns[i].GetID() } - col, err := tableDesc.FindColumnWithID(columnIDs[0]) + col, err := catalog.MustFindColumnByID(tableDesc, columnIDs[0]) if err != nil { return nil, err } @@ -399,7 +398,7 @@ func createStatsDefaultColumns( // ID if they have not already been added. Histogram stats are collected for // every indexed column. addIndexColumnStatsIfNotExists := func(colID descpb.ColumnID, isInverted bool) error { - col, err := desc.FindColumnWithID(colID) + col, err := catalog.MustFindColumnByID(desc, colID) if err != nil { return err } @@ -486,7 +485,7 @@ func createStatsDefaultColumns( colIDs := make([]descpb.ColumnID, 0, j+1) for k := 0; k <= j; k++ { - col, err := desc.FindColumnWithID(idx.GetKeyColumnID(k)) + col, err := catalog.MustFindColumnByID(desc, idx.GetKeyColumnID(k)) if err != nil { return nil, err } @@ -529,7 +528,7 @@ func createStatsDefaultColumns( // Generate stats for each column individually. for _, colID := range colIDs.Ordered() { - col, err := desc.FindColumnWithID(colID) + col, err := catalog.MustFindColumnByID(desc, colID) if err != nil { return nil, err } diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go index 9b709dbb793a..f2b66802fe44 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -919,7 +919,7 @@ func ResolveFK( } } - referencedCols, err := tabledesc.FindPublicColumnsWithNames(target, referencedColNames) + referencedCols, err := catalog.MustFindPublicColumnsByNameList(target, referencedColNames) if err != nil { return err } @@ -1044,7 +1044,7 @@ func ResolveFK( return errors.HandleAsAssertionFailure(err) } // Ensure that there is a unique constraint on the referenced side to use. - _, err = tabledesc.FindFKReferencedUniqueConstraint(target, c.(catalog.ForeignKeyConstraint)) + _, err = catalog.FindFKReferencedUniqueConstraint(target, c.(catalog.ForeignKeyConstraint)) return err } @@ -1076,7 +1076,9 @@ func CreatePartitioning( ctx, st, evalCtx, - tableDesc.FindColumnWithName, + func(name tree.Name) (catalog.Column, error) { + return catalog.MustFindColumnByTreeName(tableDesc, name) + }, int(indexDesc.Partitioning.NumImplicitColumns), indexDesc.KeyColumnNames, partBy, @@ -1726,7 +1728,7 @@ func NewTableDesc( if err != nil { return nil, err } - col, err := desc.FindColumnWithName(d.Name) + col, err := catalog.MustFindColumnByTreeName(&desc, d.Name) if err != nil { return nil, err } @@ -1787,7 +1789,7 @@ func NewTableDesc( return nil, err } if d.Inverted { - column, err := desc.FindColumnWithName(tree.Name(idx.InvertedColumnName())) + column, err := catalog.MustFindColumnByName(&desc, idx.InvertedColumnName()) if err != nil { return nil, err } @@ -2601,7 +2603,7 @@ func replaceLikeTableOpts(n *tree.CreateTable, params runParams) (tree.TableDefs Column: tree.Name(name), Direction: tree.Ascending, } - col, err := td.FindColumnWithID(idx.GetKeyColumnID(j)) + col, err := catalog.MustFindColumnByID(td, idx.GetKeyColumnID(j)) if err != nil { return nil, err } @@ -2732,7 +2734,7 @@ func setSequenceOwner( return errors.Errorf("%s is not a sequence", seqDesc.Name) } - col, err := table.FindColumnWithName(colName) + col, err := catalog.MustFindColumnByTreeName(table, colName) if err != nil { return err } @@ -2815,7 +2817,7 @@ func isImplicitlyCreatedBySystem(td *tabledesc.Mutable, c *descpb.ColumnDescript if td.IsPrimaryIndexDefaultRowID() && c.ID == td.GetPrimaryIndex().GetKeyColumnID(0) { return true, nil } - col, err := td.FindColumnWithID(c.ID) + col, err := catalog.MustFindColumnByID(td, c.ID) if err != nil { return false, err } diff --git a/pkg/sql/descriptor_mutation_test.go b/pkg/sql/descriptor_mutation_test.go index 387cb811fc9b..5c936dbf7df1 100644 --- a/pkg/sql/descriptor_mutation_test.go +++ b/pkg/sql/descriptor_mutation_test.go @@ -27,7 +27,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" "github.com/cockroachdb/cockroach/pkg/sql/catalog/lease" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/tests" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" @@ -103,7 +102,7 @@ func (mt mutationTest) makeMutationsActive(ctx context.Context) { func (mt mutationTest) writeColumnMutation( ctx context.Context, column string, m descpb.DescriptorMutation, ) { - col, err := mt.tableDesc.FindColumnWithName(tree.Name(column)) + col, err := catalog.MustFindColumnByName(mt.tableDesc, column) if err != nil { mt.Fatal(err) } diff --git a/pkg/sql/distsql_physical_planner.go b/pkg/sql/distsql_physical_planner.go index c698fe92b3d6..ac998abd6294 100644 --- a/pkg/sql/distsql_physical_planner.go +++ b/pkg/sql/distsql_physical_planner.go @@ -2816,7 +2816,7 @@ func (dsp *DistSQLPlanner) createPlanForInvertedJoin( return nil, err } - invCol, err := n.table.desc.FindColumnWithID(n.table.index.InvertedColumnID()) + invCol, err := catalog.MustFindColumnByID(n.table.desc, n.table.index.InvertedColumnID()) if err != nil { return nil, err } diff --git a/pkg/sql/distsql_plan_changefeed.go b/pkg/sql/distsql_plan_changefeed.go index 0652d25312d3..e452964d3dcb 100644 --- a/pkg/sql/distsql_plan_changefeed.go +++ b/pkg/sql/distsql_plan_changefeed.go @@ -22,6 +22,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/execinfra" "github.com/cockroachdb/cockroach/pkg/sql/opt/cat" "github.com/cockroachdb/cockroach/pkg/sql/parser" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/catid" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -512,19 +514,16 @@ func (d *familyTableDescriptor) EnforcedUniqueConstraintsWithoutIndex() []catalo // FindColumnWithID implements catalog.TableDescriptor and provides // access to extra CDC columns. func (d *familyTableDescriptor) FindColumnWithID(id descpb.ColumnID) (catalog.Column, error) { - c, err := d.TableDescriptor.FindColumnWithID(id) - if err != nil { - for _, extra := range d.extraCols { - if extra.GetID() == id { - c = extra - break - } - } - if c != nil { - err = nil + c := catalog.FindColumnByID(d.TableDescriptor, id) + if c != nil { + return c, nil + } + for _, extra := range d.extraCols { + if extra.GetID() == id { + return c, nil } } - return c, err + return nil, pgerror.Newf(pgcode.UndefinedColumn, "column-id \"%d\" does not exist", id) } // EnforcedCheckConstraints implements catalog.TableDescriptor interface. diff --git a/pkg/sql/distsql_plan_changefeed_test.go b/pkg/sql/distsql_plan_changefeed_test.go index b3da55efc03d..55e4e33bfe98 100644 --- a/pkg/sql/distsql_plan_changefeed_test.go +++ b/pkg/sql/distsql_plan_changefeed_test.go @@ -586,7 +586,7 @@ FAMILY extra (extra) var inputTypes []*types.T // We target main family; so only 3 columns should be used. for _, id := range fooDesc.GetFamilies()[0].ColumnIDs { - col, err := fooDesc.FindColumnWithID(id) + col, err := catalog.MustFindColumnByID(fooDesc, id) require.NoError(t, err) inputCols.Set(col.GetID(), len(inputTypes)) inputTypes = append(inputTypes, col.GetType()) @@ -690,7 +690,7 @@ func mkPkKey(t *testing.T, tableID descpb.ID, vals ...int) roachpb.Key { func copyColumnAs(t *testing.T, table catalog.TableDescriptor, from, to tree.Name) catalog.Column { t.Helper() - src, err := table.FindColumnWithName(from) + src, err := catalog.MustFindColumnByTreeName(table, from) require.NoError(t, err) dst := src.DeepCopy() desc := dst.ColumnDesc() diff --git a/pkg/sql/distsql_plan_stats.go b/pkg/sql/distsql_plan_stats.go index 93b04e5b1a6c..8d1b3d98bb3c 100644 --- a/pkg/sql/distsql_plan_stats.go +++ b/pkg/sql/distsql_plan_stats.go @@ -188,7 +188,7 @@ func (dsp *DistSQLPlanner) createPartialStatsPlan( return nil, err } - column, err := desc.FindColumnWithID(reqStat.columns[0]) + column, err := catalog.MustFindColumnByID(desc, reqStat.columns[0]) if err != nil { return nil, err } diff --git a/pkg/sql/drop_index.go b/pkg/sql/drop_index.go index 1ca81534a3c5..3f7db4ca162e 100644 --- a/pkg/sql/drop_index.go +++ b/pkg/sql/drop_index.go @@ -144,7 +144,7 @@ func (n *dropIndexNode) startExec(params runParams) error { if idx != nil { for i, count := 0, idx.NumKeyColumns(); i < count; i++ { id := idx.GetKeyColumnID(i) - col, err := tableDesc.FindColumnWithID(id) + col, err := catalog.MustFindColumnByID(tableDesc, id) if err != nil { return err } @@ -166,7 +166,7 @@ func (n *dropIndexNode) startExec(params runParams) error { if shardColName != "" { // Retrieve the sharded column descriptor by name. - shardColDesc, err := tableDesc.FindColumnWithName(tree.Name(shardColName)) + shardColDesc, err := catalog.MustFindColumnByName(tableDesc, shardColName) if err != nil { return err } diff --git a/pkg/sql/drop_test.go b/pkg/sql/drop_test.go index 8bad2ef0ea9f..2bd249bf9d5b 100644 --- a/pkg/sql/drop_test.go +++ b/pkg/sql/drop_test.go @@ -1220,7 +1220,7 @@ func TestDropIndexOnHashShardedIndexWithStoredShardColumn(t *testing.T) { require.NoError(t, err) require.True(t, shardIdx.IsSharded()) require.Equal(t, "crdb_internal_a_shard_7", shardIdx.GetShardColumnName()) - shardCol, err := tableDesc.FindColumnWithName("crdb_internal_a_shard_7") + shardCol, err := catalog.MustFindColumnByName(tableDesc, "crdb_internal_a_shard_7") require.NoError(t, err) require.False(t, shardCol.IsVirtual()) @@ -1236,7 +1236,7 @@ func TestDropIndexOnHashShardedIndexWithStoredShardColumn(t *testing.T) { })) _, err = catalog.MustFindIndexByName(tableDesc, "idx") require.Error(t, err) - shardCol, err = tableDesc.FindColumnWithName("crdb_internal_a_shard_7") + shardCol, err = catalog.MustFindColumnByTreeName(tableDesc, "crdb_internal_a_shard_7") require.NoError(t, err) require.False(t, shardCol.IsVirtual()) diff --git a/pkg/sql/evalcatalog/encode_table_index_key.go b/pkg/sql/evalcatalog/encode_table_index_key.go index 82aaa0aed584..562a383afc97 100644 --- a/pkg/sql/evalcatalog/encode_table_index_key.go +++ b/pkg/sql/evalcatalog/encode_table_index_key.go @@ -66,7 +66,7 @@ func (ec *Builtins) EncodeTableIndexKey( var extraColNames []string for i := 0; i < index.NumKeySuffixColumns(); i++ { id := index.GetKeySuffixColumnID(i) - col, colErr := tableDesc.FindColumnWithID(id) + col, colErr := catalog.MustFindColumnByID(tableDesc, id) if colErr != nil { return nil, errors.CombineErrors(err, colErr) } @@ -74,7 +74,7 @@ func (ec *Builtins) EncodeTableIndexKey( } var allColNames []string for _, id := range indexColIDs { - col, colErr := tableDesc.FindColumnWithID(id) + col, colErr := catalog.MustFindColumnByID(tableDesc, id) if colErr != nil { return nil, errors.CombineErrors(err, colErr) } @@ -100,7 +100,7 @@ func (ec *Builtins) EncodeTableIndexKey( // types of the index columns. So, try to cast the input datums to // the types of the index columns here. var newDatum tree.Datum - col, err := tableDesc.FindColumnWithID(indexColIDs[i]) + col, err := catalog.MustFindColumnByID(tableDesc, indexColIDs[i]) if err != nil { return nil, err } diff --git a/pkg/sql/evalcatalog/pg_updatable.go b/pkg/sql/evalcatalog/pg_updatable.go index f7e7da8f1319..086e56d37c61 100644 --- a/pkg/sql/evalcatalog/pg_updatable.go +++ b/pkg/sql/evalcatalog/pg_updatable.go @@ -13,6 +13,7 @@ package evalcatalog import ( "context" + "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/redact" @@ -136,15 +137,11 @@ func (b *Builtins) PGColumnIsUpdatable( return tree.DBoolFalse, nil } - column, err := tableDesc.FindColumnWithPGAttributeNum(attNum) - if err != nil { - if sqlerrors.IsUndefinedColumnError(err) { - // When column does not exist postgres returns true. - return tree.DBoolTrue, nil - } - return nil, err + column := catalog.FindColumnByPGAttributeNum(tableDesc, attNum) + if column == nil { + // When column does not exist postgres returns true. + return tree.DBoolTrue, nil } - // pg_column_is_updatable was created for compatibility. This // will return true if is a table (not virtual) and column is not // a computed column. diff --git a/pkg/sql/importer/import_planning.go b/pkg/sql/importer/import_planning.go index 3bacf71cc405..0de0cd2c75d9 100644 --- a/pkg/sql/importer/import_planning.go +++ b/pkg/sql/importer/import_planning.go @@ -811,7 +811,7 @@ func importPlanHook( var intoCols []string isTargetCol := make(map[string]bool) for _, name := range importStmt.IntoCols { - active, err := tabledesc.FindPublicColumnsWithNames(found, tree.NameList{name}) + active, err := catalog.MustFindPublicColumnsByNameList(found, tree.NameList{name}) if err != nil { return errors.Wrap(err, "verifying target columns") } diff --git a/pkg/sql/information_schema.go b/pkg/sql/information_schema.go index b6cdaaceaf3b..64f20eaa662a 100644 --- a/pkg/sql/information_schema.go +++ b/pkg/sql/information_schema.go @@ -27,7 +27,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/nstree" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/isql" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" @@ -923,7 +922,7 @@ https://www.postgresql.org/docs/9.5/infoschema-referential-constraints.html`, if r, ok := matchOptionMap[fk.Match()]; ok { matchType = r } - refConstraint, err := tabledesc.FindFKReferencedUniqueConstraint(refTable, fk) + refConstraint, err := catalog.FindFKReferencedUniqueConstraint(refTable, fk) if err != nil { return err } diff --git a/pkg/sql/opt_catalog.go b/pkg/sql/opt_catalog.go index e8572adeca43..d5c98613e7fc 100644 --- a/pkg/sql/opt_catalog.go +++ b/pkg/sql/opt_catalog.go @@ -836,7 +836,7 @@ func newOptTable( // This check is done for upgrade purposes. We need to avoid adding the // system column if the table has a column with this name for some reason. for _, sysCol := range ot.desc.SystemColumns() { - found, _ := desc.FindColumnWithName(sysCol.ColName()) + found := catalog.FindColumnByTreeName(desc, sysCol.ColName()) if found == nil || found.IsSystemColumn() { col, ord := newColumn() cd := sysCol.ColumnDesc() diff --git a/pkg/sql/pg_catalog.go b/pkg/sql/pg_catalog.go index 1899e951afa2..487111247cdd 100644 --- a/pkg/sql/pg_catalog.go +++ b/pkg/sql/pg_catalog.go @@ -945,7 +945,7 @@ func populateTableConstraints( if err != nil { return err } - if refConstraint, err := tabledesc.FindFKReferencedUniqueConstraint(referencedTable, fk); err != nil { + if refConstraint, err := catalog.FindFKReferencedUniqueConstraint(referencedTable, fk); err != nil { // We couldn't find a unique constraint that matched. This shouldn't // happen. log.Warningf(ctx, "broken fk reference: %v", err) @@ -1505,7 +1505,7 @@ https://www.postgresql.org/docs/9.5/catalog-pg-depend.html`, return err } refObjID := oidZero - if refConstraint, err := tabledesc.FindFKReferencedUniqueConstraint(referencedTable, fk); err != nil { + if refConstraint, err := catalog.FindFKReferencedUniqueConstraint(referencedTable, fk); err != nil { // We couldn't find a unique constraint that matched. This shouldn't // happen. log.Warningf(ctx, "broken fk reference: %v", err) @@ -1848,7 +1848,7 @@ https://www.postgresql.org/docs/9.5/catalog-pg-index.html`, exprs := make([]string, 0, index.NumKeyColumns()) for i := index.IndexDesc().ExplicitColumnStartIdx(); i < index.NumKeyColumns(); i++ { columnID := index.GetKeyColumnID(i) - col, err := table.FindColumnWithID(columnID) + col, err := catalog.MustFindColumnByID(table, columnID) if err != nil { return err } diff --git a/pkg/sql/randgen/schema.go b/pkg/sql/randgen/schema.go index 3d1543fbaeb1..617aef09fdeb 100644 --- a/pkg/sql/randgen/schema.go +++ b/pkg/sql/randgen/schema.go @@ -729,7 +729,7 @@ func TestingMakePrimaryIndexKeyForTenant( } // Check that the value type matches. colID := index.GetKeyColumnID(i) - col, _ := desc.FindColumnWithID(colID) + col := catalog.FindColumnByID(desc, colID) if col != nil && col.Public() { colTyp := datums[i].ResolvedType() if t := colTyp.Family(); t != col.GetType().Family() { @@ -785,7 +785,7 @@ func TestingMakeSecondaryIndexKey( } // Check that the value type matches. colID := index.GetKeyColumnID(i) - col, _ := desc.FindColumnWithID(colID) + col := catalog.FindColumnByID(desc, colID) if col != nil && col.Public() { colTyp := datums[i].ResolvedType() if t := colTyp.Family(); t != col.GetType().Family() { diff --git a/pkg/sql/rename_column.go b/pkg/sql/rename_column.go index 9f31d1962dbc..1a9b6569fc2f 100644 --- a/pkg/sql/rename_column.go +++ b/pkg/sql/rename_column.go @@ -96,7 +96,7 @@ func (p *planner) findColumnToRename( return nil, errEmptyColumnName } - col, err := tableDesc.FindColumnWithName(oldName) + col, err := catalog.MustFindColumnByTreeName(tableDesc, oldName) if err != nil { return nil, err } diff --git a/pkg/sql/resolver.go b/pkg/sql/resolver.go index 3ffbb692bf5f..5b286d9a59eb 100644 --- a/pkg/sql/resolver.go +++ b/pkg/sql/resolver.go @@ -267,7 +267,7 @@ func validateColumnForHasPrivilegeSpecifier( table catalog.TableDescriptor, specifier eval.HasPrivilegeSpecifier, ) error { if specifier.ColumnName != nil { - _, err := table.FindColumnWithName(*specifier.ColumnName) + _, err := catalog.MustFindColumnByTreeName(table, *specifier.ColumnName) return err } if specifier.ColumnAttNum != nil { diff --git a/pkg/sql/row/deleter.go b/pkg/sql/row/deleter.go index 3646c8eb067c..6240888d9439 100644 --- a/pkg/sql/row/deleter.go +++ b/pkg/sql/row/deleter.go @@ -60,7 +60,7 @@ func MakeDeleter( } else { maybeAddCol := func(colID descpb.ColumnID) error { if _, ok := fetchColIDtoRowIndex.Get(colID); !ok { - col, err := tableDesc.FindColumnWithID(colID) + col, err := catalog.MustFindColumnByID(tableDesc, colID) if err != nil { return err } diff --git a/pkg/sql/row/errors.go b/pkg/sql/row/errors.go index c2b52fe02638..98e5fc20ffde 100644 --- a/pkg/sql/row/errors.go +++ b/pkg/sql/row/errors.go @@ -247,7 +247,7 @@ func DecodeRowInfo( } cols := make([]catalog.Column, len(colIDs)) for i, colID := range colIDs { - col, err := tableDesc.FindColumnWithID(colID) + col, err := catalog.MustFindColumnByID(tableDesc, colID) if err != nil { return nil, nil, nil, err } diff --git a/pkg/sql/rowenc/index_encoding.go b/pkg/sql/rowenc/index_encoding.go index 4406d8fa31da..eaf59f1fe03b 100644 --- a/pkg/sql/rowenc/index_encoding.go +++ b/pkg/sql/rowenc/index_encoding.go @@ -1080,7 +1080,7 @@ func EncodePrimaryIndex( // We want to include this column if its value is non-null or // we were requested to include all of the columns. if datum != tree.DNull || includeEmpty { - col, err := tableDesc.FindColumnWithID(family.DefaultColumnID) + col, err := catalog.MustFindColumnByID(tableDesc, family.DefaultColumnID) if err != nil { return err } diff --git a/pkg/sql/rowenc/index_fetch.go b/pkg/sql/rowenc/index_fetch.go index 75f7fbf74d16..6633df77e645 100644 --- a/pkg/sql/rowenc/index_fetch.go +++ b/pkg/sql/rowenc/index_fetch.go @@ -76,7 +76,7 @@ func InitIndexFetchSpec( s.FetchedColumns = make([]fetchpb.IndexFetchSpec_Column, len(fetchColumnIDs)) } for i, colID := range fetchColumnIDs { - col, err := table.FindColumnWithID(colID) + col, err := catalog.MustFindColumnByID(table, colID) if err != nil { return err } diff --git a/pkg/sql/rowenc/index_fetch_test.go b/pkg/sql/rowenc/index_fetch_test.go index 922bb738e6bf..6efba4041930 100644 --- a/pkg/sql/rowenc/index_fetch_test.go +++ b/pkg/sql/rowenc/index_fetch_test.go @@ -22,7 +22,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" "github.com/cockroachdb/cockroach/pkg/sql/catalog/fetchpb" "github.com/cockroachdb/cockroach/pkg/sql/rowenc" - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/testutils/datapathutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" @@ -67,7 +66,7 @@ func TestInitIndexFetchSpec(t *testing.T) { fetchColumnIDs := make([]descpb.ColumnID, len(params.Columns)) for i, name := range params.Columns { - col, err := table.FindColumnWithName(tree.Name(name)) + col, err := catalog.MustFindColumnByName(table, name) if err != nil { d.Fatalf(t, "%+v", err) } diff --git a/pkg/sql/rowenc/partition.go b/pkg/sql/rowenc/partition.go index 48ed98d27c81..8bbf3dc076b1 100644 --- a/pkg/sql/rowenc/partition.go +++ b/pkg/sql/rowenc/partition.go @@ -121,7 +121,7 @@ func DecodePartitionTuple( for i := len(prefixDatums); i < index.NumKeyColumns() && i < len(prefixDatums)+part.NumColumns(); i++ { colID := index.GetKeyColumnID(i) - col, err := tableDesc.FindColumnWithID(colID) + col, err := catalog.MustFindColumnByID(tableDesc, colID) if err != nil { return nil, nil, err } diff --git a/pkg/sql/rowexec/inverted_joiner_test.go b/pkg/sql/rowexec/inverted_joiner_test.go index 6d6e6443cb7d..6e08485eeffc 100644 --- a/pkg/sql/rowexec/inverted_joiner_test.go +++ b/pkg/sql/rowexec/inverted_joiner_test.go @@ -19,6 +19,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/settings/cluster" + "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" "github.com/cockroachdb/cockroach/pkg/sql/catalog/fetchpb" @@ -667,7 +668,7 @@ func TestInvertedJoiner(t *testing.T) { for _, c := range td.IndexFullColumns(index) { fetchColIDs = append(fetchColIDs, c.GetID()) } - invCol, err := td.FindColumnWithID(index.InvertedColumnID()) + invCol, err := catalog.MustFindColumnByID(td, index.InvertedColumnID()) if err != nil { t.Fatal(err) } diff --git a/pkg/sql/rowexec/utils_test.go b/pkg/sql/rowexec/utils_test.go index 5e819397278e..2197db5ed6f5 100644 --- a/pkg/sql/rowexec/utils_test.go +++ b/pkg/sql/rowexec/utils_test.go @@ -28,7 +28,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/rowcontainer" "github.com/cockroachdb/cockroach/pkg/sql/rowenc" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/testutils/distsqlutils" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" @@ -208,7 +207,7 @@ func makeFetchSpec( var colIDs []descpb.ColumnID if colNames != "" { for _, col := range strings.Split(colNames, ",") { - col, err := table.FindColumnWithName(tree.Name(col)) + col, err := catalog.MustFindColumnByName(table, col) if err != nil { t.Fatal(err) } diff --git a/pkg/sql/scan.go b/pkg/sql/scan.go index e07df055a1c7..3a2b345adcfc 100644 --- a/pkg/sql/scan.go +++ b/pkg/sql/scan.go @@ -198,7 +198,7 @@ func initColsForScan( cols = make([]catalog.Column, len(colCfg.wantedColumns)) for i, colID := range colCfg.wantedColumns { - col, err := desc.FindColumnWithID(colID) + col, err := catalog.MustFindColumnByID(desc, colID) if err != nil { return cols, err } diff --git a/pkg/sql/scatter.go b/pkg/sql/scatter.go index 6ebef5c64e90..f5d51f9aef40 100644 --- a/pkg/sql/scatter.go +++ b/pkg/sql/scatter.go @@ -16,6 +16,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/keys" "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" @@ -65,7 +66,7 @@ func (p *planner) Scatter(ctx context.Context, n *tree.Scatter) (planNode, error desiredTypes := make([]*types.T, index.NumKeyColumns()) for i := 0; i < index.NumKeyColumns(); i++ { colID := index.GetKeyColumnID(i) - c, err := tableDesc.FindColumnWithID(colID) + c, err := catalog.MustFindColumnByID(tableDesc, colID) if err != nil { return nil, err } diff --git a/pkg/sql/schema_changer.go b/pkg/sql/schema_changer.go index 2a74e5cd5e11..0c503dd35817 100644 --- a/pkg/sql/schema_changer.go +++ b/pkg/sql/schema_changer.go @@ -1567,7 +1567,7 @@ func (sc *SchemaChanger) done(ctx context.Context) error { // If we are adding a new REGIONAL BY ROW column, after backfilling, the // default expression should be switched to utilize to gateway_region. if colID := lcSwap.NewRegionalByRowColumnID; colID != nil { - col, err := scTable.FindColumnWithID(*colID) + col, err := catalog.MustFindColumnByID(scTable, *colID) if err != nil { return err } diff --git a/pkg/sql/schemachanger/scbuild/builder_state.go b/pkg/sql/schemachanger/scbuild/builder_state.go index 4a63f264619f..8bd17ce1cad7 100644 --- a/pkg/sql/schemachanger/scbuild/builder_state.go +++ b/pkg/sql/schemachanger/scbuild/builder_state.go @@ -343,7 +343,9 @@ func (b *builderState) IndexPartitioningDescriptor( b.ctx, b.clusterSettings, b.evalCtx, - tbl.FindColumnWithName, + func(name tree.Name) (catalog.Column, error) { + return catalog.MustFindColumnByTreeName(tbl, name) + }, oldNumImplicitColumns, oldKeyColumnNames, partBy, diff --git a/pkg/sql/schemachanger/scexec/exec_backfill_test.go b/pkg/sql/schemachanger/scexec/exec_backfill_test.go index cf2d27b5ebf3..4ec7967ef0d9 100644 --- a/pkg/sql/schemachanger/scexec/exec_backfill_test.go +++ b/pkg/sql/schemachanger/scexec/exec_backfill_test.go @@ -24,7 +24,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scdeps/sctestdeps" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scexec" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scop" - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" "github.com/cockroachdb/cockroach/pkg/util/hlc" @@ -68,7 +67,7 @@ func TestExecBackfiller(t *testing.T) { var columnIDs, keySuffixColumnIDs []descpb.ColumnID var columnIDSet catalog.TableColSet for _, c := range columns { - col, err := mut.FindColumnWithName(tree.Name(c)) + col, err := catalog.MustFindColumnByName(mut, c) require.NoError(t, err) dirs = append(dirs, catenumpb.IndexColumn_ASC) columnIDs = append(columnIDs, col.GetID()) diff --git a/pkg/sql/schemachanger/scexec/scmutationexec/column.go b/pkg/sql/schemachanger/scexec/scmutationexec/column.go index 94b41076a028..9457f91a1c95 100644 --- a/pkg/sql/schemachanger/scexec/scmutationexec/column.go +++ b/pkg/sql/schemachanger/scexec/scmutationexec/column.go @@ -238,7 +238,7 @@ func (m *visitor) SetColumnName(ctx context.Context, op scop.SetColumnName) erro if err != nil { return err } - col, err := tbl.FindColumnWithID(op.ColumnID) + col, err := catalog.MustFindColumnByID(tbl, op.ColumnID) if err != nil { return errors.AssertionFailedf("column %d not found in table %q (%d)", op.ColumnID, tbl.GetName(), tbl.GetID()) } @@ -252,7 +252,7 @@ func (m *visitor) AddColumnDefaultExpression( if err != nil { return err } - col, err := tbl.FindColumnWithID(op.Default.ColumnID) + col, err := catalog.MustFindColumnByID(tbl, op.Default.ColumnID) if err != nil { return err } @@ -277,7 +277,7 @@ func (m *visitor) RemoveColumnDefaultExpression( if err != nil || tbl.Dropped() { return err } - col, err := tbl.FindColumnWithID(op.ColumnID) + col, err := catalog.MustFindColumnByID(tbl, op.ColumnID) if err != nil { return err } @@ -293,7 +293,7 @@ func (m *visitor) AddColumnOnUpdateExpression( if err != nil { return err } - col, err := tbl.FindColumnWithID(op.OnUpdate.ColumnID) + col, err := catalog.MustFindColumnByID(tbl, op.OnUpdate.ColumnID) if err != nil { return err } @@ -318,7 +318,7 @@ func (m *visitor) RemoveColumnOnUpdateExpression( if err != nil || tbl.Dropped() { return err } - col, err := tbl.FindColumnWithID(op.ColumnID) + col, err := catalog.MustFindColumnByID(tbl, op.ColumnID) if err != nil { return err } diff --git a/pkg/sql/schemachanger/scexec/scmutationexec/eventlog.go b/pkg/sql/schemachanger/scexec/scmutationexec/eventlog.go index da8ebf203237..f7e282f5400c 100644 --- a/pkg/sql/schemachanger/scexec/scmutationexec/eventlog.go +++ b/pkg/sql/schemachanger/scexec/scmutationexec/eventlog.go @@ -152,7 +152,7 @@ func asCommentEventPayload( if err != nil { return nil, err } - col, err := tbl.FindColumnWithID(e.ColumnID) + col, err := catalog.MustFindColumnByID(tbl, e.ColumnID) if err != nil { return nil, err } diff --git a/pkg/sql/schemachanger/scexec/scmutationexec/index.go b/pkg/sql/schemachanger/scexec/scmutationexec/index.go index f500de0e7e23..0412c086fe4e 100644 --- a/pkg/sql/schemachanger/scexec/scmutationexec/index.go +++ b/pkg/sql/schemachanger/scexec/scmutationexec/index.go @@ -359,7 +359,7 @@ func (m *visitor) AddColumnToIndex(ctx context.Context, op scop.AddColumnToIndex if err != nil { return err } - column, err := tbl.FindColumnWithID(op.ColumnID) + column, err := catalog.MustFindColumnByID(tbl, op.ColumnID) if err != nil { return err } @@ -430,7 +430,7 @@ func (m *visitor) RemoveColumnFromIndex(ctx context.Context, op scop.RemoveColum if err != nil { return err } - column, err := tbl.FindColumnWithID(op.ColumnID) + column, err := catalog.MustFindColumnByID(tbl, op.ColumnID) if err != nil { return err } diff --git a/pkg/sql/schemachanger/scexec/scmutationexec/references.go b/pkg/sql/schemachanger/scexec/scmutationexec/references.go index 5f2f34ffed9a..68c73c304f55 100644 --- a/pkg/sql/schemachanger/scexec/scmutationexec/references.go +++ b/pkg/sql/schemachanger/scexec/scmutationexec/references.go @@ -50,7 +50,7 @@ func (m *visitor) RemoveSequenceOwner(ctx context.Context, op scop.RemoveSequenc if err != nil || tbl.Dropped() { return err } - col, err := tbl.FindColumnWithID(op.ColumnID) + col, err := catalog.MustFindColumnByID(tbl, op.ColumnID) if err != nil || col == nil { return err } @@ -214,7 +214,7 @@ func (m *visitor) UpdateBackReferencesInSequences( return err } if op.BackReferencedColumnID != 0 { - col, err := tbl.FindColumnWithID(op.BackReferencedColumnID) + col, err := catalog.MustFindColumnByID(tbl, op.BackReferencedColumnID) if err != nil { return err } diff --git a/pkg/sql/scrub_fk.go b/pkg/sql/scrub_fk.go index 2de520f82f99..1661f8bc784a 100644 --- a/pkg/sql/scrub_fk.go +++ b/pkg/sql/scrub_fk.go @@ -16,7 +16,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" - "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/scrub" "github.com/cockroachdb/cockroach/pkg/sql/sem/semenumpb" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" @@ -142,7 +141,7 @@ func (o *sqlForeignKeyCheckOperation) Next(params runParams) (tree.Datums, error for i, n := 0, o.constraint.NumOriginColumns(); i < n; i++ { id := o.constraint.GetOriginColumnID(i) idx := o.colIDToRowIdx.GetDefault(id) - col, err := tabledesc.FindPublicColumnWithID(o.tableDesc, id) + col, err := catalog.MustFindPublicColumnByID(o.tableDesc, id) if err != nil { return nil, err } @@ -153,7 +152,7 @@ func (o *sqlForeignKeyCheckOperation) Next(params runParams) (tree.Datums, error id := o.tableDesc.GetPrimaryIndex().GetKeyColumnID(i) if !originColumnIDs.Contains(id) { idx := o.colIDToRowIdx.GetDefault(id) - col, err := tabledesc.FindPublicColumnWithID(o.tableDesc, id) + col, err := catalog.MustFindPublicColumnByID(o.tableDesc, id) if err != nil { return nil, err } diff --git a/pkg/sql/scrub_unique_constraint.go b/pkg/sql/scrub_unique_constraint.go index e36128e371ac..92bbc5174a25 100644 --- a/pkg/sql/scrub_unique_constraint.go +++ b/pkg/sql/scrub_unique_constraint.go @@ -107,7 +107,7 @@ func (o *sqlUniqueConstraintCheckOperation) Start(params runParams) error { keyCols := make([]string, len(o.cols)) matchers := make([]string, len(o.cols)) for i := 0; i < len(o.cols); i++ { - col, err := o.tableDesc.FindColumnWithID(o.cols[i]) + col, err := catalog.MustFindColumnByID(o.tableDesc, o.cols[i]) if err != nil { return err } diff --git a/pkg/sql/sequence.go b/pkg/sql/sequence.go index b7589d8517d9..a261c41742bb 100644 --- a/pkg/sql/sequence.go +++ b/pkg/sql/sequence.go @@ -527,7 +527,7 @@ func removeSequenceOwnerIfExists( if tableDesc.Dropped() { return nil } - col, err := tableDesc.FindColumnWithID(opts.SequenceOwner.OwnerColumnID) + col, err := catalog.MustFindColumnByID(tableDesc, opts.SequenceOwner.OwnerColumnID) if err != nil { return err } @@ -568,7 +568,7 @@ func resolveColumnItemToDescriptors( if err != nil { return nil, nil, err } - col, err := tableDesc.FindColumnWithName(columnItem.ColumnName) + col, err := catalog.MustFindColumnByTreeName(tableDesc, columnItem.ColumnName) if err != nil { return nil, nil, err } diff --git a/pkg/sql/show_create_clauses.go b/pkg/sql/show_create_clauses.go index 3087d2bf229a..9259ceb78ff3 100644 --- a/pkg/sql/show_create_clauses.go +++ b/pkg/sql/show_create_clauses.go @@ -377,7 +377,7 @@ func showComments( } for _, columnComment := range tc.columns { - col, err := table.FindColumnWithPGAttributeNum(descpb.PGAttributeNum(columnComment.subID)) + col, err := catalog.MustFindColumnByPGAttributeNum(table, descpb.PGAttributeNum(columnComment.subID)) if err != nil { return err } @@ -532,7 +532,7 @@ func showFamilyClause(desc catalog.TableDescriptor, f *tree.FmtCtx) { for _, fam := range families { activeColumnNames := make([]string, 0, len(fam.ColumnNames)) for i, colID := range fam.ColumnIDs { - if col, _ := desc.FindColumnWithID(colID); col != nil && col.Public() { + if col := catalog.FindColumnByID(desc, colID); col != nil && col.Public() { activeColumnNames = append(activeColumnNames, fam.ColumnNames[i]) } } diff --git a/pkg/sql/show_fingerprints.go b/pkg/sql/show_fingerprints.go index cd6b4bf02fb0..acff3d23f80a 100644 --- a/pkg/sql/show_fingerprints.go +++ b/pkg/sql/show_fingerprints.go @@ -115,21 +115,21 @@ func (n *showFingerprintsNode) Next(params runParams) (bool, error) { } } else { for i := 0; i < index.NumKeyColumns(); i++ { - col, err := n.tableDesc.FindColumnWithID(index.GetKeyColumnID(i)) + col, err := catalog.MustFindColumnByID(n.tableDesc, index.GetKeyColumnID(i)) if err != nil { return false, err } addColumn(col) } for i := 0; i < index.NumKeySuffixColumns(); i++ { - col, err := n.tableDesc.FindColumnWithID(index.GetKeySuffixColumnID(i)) + col, err := catalog.MustFindColumnByID(n.tableDesc, index.GetKeySuffixColumnID(i)) if err != nil { return false, err } addColumn(col) } for i := 0; i < index.NumSecondaryStoredColumns(); i++ { - col, err := n.tableDesc.FindColumnWithID(index.GetStoredColumnID(i)) + col, err := catalog.MustFindColumnByID(n.tableDesc, index.GetStoredColumnID(i)) if err != nil { return false, err } diff --git a/pkg/sql/show_stats.go b/pkg/sql/show_stats.go index b7431e6efec5..d071a35aac2b 100644 --- a/pkg/sql/show_stats.go +++ b/pkg/sql/show_stats.go @@ -68,18 +68,14 @@ var showTableStatsOptValidate = map[string]exprutil.KVStringOptValidate{ showTableStatsOptMerge: exprutil.KVStringOptRequireNoValue, } -func containsDroppedColumn(colIDs tree.Datums, desc catalog.TableDescriptor) (bool, error) { +func containsDroppedColumn(colIDs tree.Datums, desc catalog.TableDescriptor) bool { for _, colID := range colIDs { cid := descpb.ColumnID(*colID.(*tree.DInt)) - if _, err := desc.FindColumnWithID(cid); err != nil { - if sqlerrors.IsUndefinedColumnError(err) { - return true, nil - } else { - return false, err - } + if catalog.FindColumnByID(desc, cid) == nil { + return true } } - return false, nil + return false } // ShowTableStats returns a SHOW STATISTICS statement for the specified table. @@ -205,10 +201,7 @@ func (p *planner) ShowTableStats(ctx context.Context, n *tree.ShowTableStats) (p for _, row := range rows { // Skip stats on dropped columns. colIDs := row[columnIDsIdx].(*tree.DArray).Array - ignoreStatsRowWithDroppedColumn, err := containsDroppedColumn(colIDs, desc) - if err != nil { - return nil, err - } + ignoreStatsRowWithDroppedColumn := containsDroppedColumn(colIDs, desc) if ignoreStatsRowWithDroppedColumn { continue } @@ -388,7 +381,7 @@ func (p *planner) ShowTableStats(ctx context.Context, n *tree.ShowTableStats) (p func statColumnString(desc catalog.TableDescriptor, colID tree.Datum) (colName string, err error) { id := descpb.ColumnID(*colID.(*tree.DInt)) - colDesc, err := desc.FindColumnWithID(id) + colDesc, err := catalog.MustFindColumnByID(desc, id) if err != nil { // This can happen if a column was removed. return "", err diff --git a/pkg/sql/tests/hash_sharded_test.go b/pkg/sql/tests/hash_sharded_test.go index 099d38980986..015eaae9a23e 100644 --- a/pkg/sql/tests/hash_sharded_test.go +++ b/pkg/sql/tests/hash_sharded_test.go @@ -19,7 +19,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -34,7 +33,7 @@ func getShardColumnID( if err != nil { t.Fatal(err) } - shardCol, err := tableDesc.FindColumnWithName(tree.Name(idx.GetShardColumnName())) + shardCol, err := catalog.MustFindColumnByName(tableDesc, idx.GetShardColumnName()) if err != nil { t.Fatal(err) } diff --git a/pkg/sql/ttl/ttljob/BUILD.bazel b/pkg/sql/ttl/ttljob/BUILD.bazel index d72816dd3477..bcdac7e3372d 100644 --- a/pkg/sql/ttl/ttljob/BUILD.bazel +++ b/pkg/sql/ttl/ttljob/BUILD.bazel @@ -24,6 +24,7 @@ go_library( "//pkg/settings", "//pkg/settings/cluster", "//pkg/sql", + "//pkg/sql/catalog", "//pkg/sql/catalog/catenumpb", "//pkg/sql/catalog/catpb", "//pkg/sql/catalog/colinfo", @@ -78,6 +79,7 @@ go_test( "//pkg/security/securitytest", "//pkg/server", "//pkg/sql", + "//pkg/sql/catalog", "//pkg/sql/catalog/colinfo", "//pkg/sql/catalog/desctestutils", "//pkg/sql/lexbase", diff --git a/pkg/sql/ttl/ttljob/ttljob_processor.go b/pkg/sql/ttl/ttljob/ttljob_processor.go index e2f101c13991..36984e51dbfe 100644 --- a/pkg/sql/ttl/ttljob/ttljob_processor.go +++ b/pkg/sql/ttl/ttljob/ttljob_processor.go @@ -18,6 +18,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/jobs" "github.com/cockroachdb/cockroach/pkg/jobs/jobspb" + "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descs" "github.com/cockroachdb/cockroach/pkg/sql/execinfra" "github.com/cockroachdb/cockroach/pkg/sql/execinfrapb" @@ -77,7 +78,7 @@ func (t *ttlProcessor) work(ctx context.Context) error { primaryIndexDesc := desc.GetPrimaryIndex().IndexDesc() pkColumns = primaryIndexDesc.KeyColumnNames for _, id := range primaryIndexDesc.KeyColumnIDs { - col, err := desc.FindColumnWithID(id) + col, err := catalog.MustFindColumnByID(desc, id) if err != nil { return err } diff --git a/pkg/sql/ttl/ttljob/ttljob_test.go b/pkg/sql/ttl/ttljob/ttljob_test.go index 912e43d9fd69..cb5173abd1a5 100644 --- a/pkg/sql/ttl/ttljob/ttljob_test.go +++ b/pkg/sql/ttl/ttljob/ttljob_test.go @@ -29,6 +29,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/scheduledjobs" "github.com/cockroachdb/cockroach/pkg/sql" + "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" "github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils" "github.com/cockroachdb/cockroach/pkg/sql/lexbase" @@ -826,7 +827,7 @@ func TestRowLevelTTLJobRandomEntries(t *testing.T) { // Note we can split a PRIMARY KEY partially. numKeyCols := 1 + rng.Intn(tbDesc.GetPrimaryIndex().NumKeyColumns()) for idx := 0; idx < numKeyCols; idx++ { - col, err := tbDesc.FindColumnWithID(tbDesc.GetPrimaryIndex().GetKeyColumnID(idx)) + col, err := catalog.MustFindColumnByID(tbDesc, tbDesc.GetPrimaryIndex().GetKeyColumnID(idx)) require.NoError(t, err) placeholders = append(placeholders, fmt.Sprintf("$%d", idx+1)) diff --git a/pkg/sql/type_change.go b/pkg/sql/type_change.go index 29dbd80fe450..42615b6ae57c 100644 --- a/pkg/sql/type_change.go +++ b/pkg/sql/type_change.go @@ -799,7 +799,7 @@ func (t *typeSchemaChanger) canRemoveEnumValue( } keyColumns := make([]catalog.Column, 0, idx.NumKeyColumns()) for i := 0; i < idx.NumKeyColumns(); i++ { - col, err := desc.FindColumnWithID(idx.GetKeyColumnID(i)) + col, err := catalog.MustFindColumnByID(desc, idx.GetKeyColumnID(i)) if err != nil { return errors.WithAssertionFailure(err) } diff --git a/pkg/upgrade/upgrades/schema_changes.go b/pkg/upgrade/upgrades/schema_changes.go index 8e0289fdef0f..d2aa9faf2499 100644 --- a/pkg/upgrade/upgrades/schema_changes.go +++ b/pkg/upgrade/upgrades/schema_changes.go @@ -20,7 +20,6 @@ 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/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/upgrade" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -178,15 +177,12 @@ func ensureProtoMessagesAreEqual(expected, found protoutil.Message) error { // expectedTable descriptor. The comparison is not strict as several descriptor // fields are ignored. func hasColumn(storedTable, expectedTable catalog.TableDescriptor, colName string) (bool, error) { - storedCol, err := storedTable.FindColumnWithName(tree.Name(colName)) - if err != nil { - if strings.Contains(err.Error(), "does not exist") { - return false, nil - } - return false, err + storedCol := catalog.FindColumnByName(storedTable, colName) + if storedCol == nil { + return false, nil } - expectedCol, err := expectedTable.FindColumnWithName(tree.Name(colName)) + expectedCol, err := catalog.MustFindColumnByName(expectedTable, colName) if err != nil { return false, errors.Wrapf(err, "columns name %s is invalid.", colName) } @@ -235,14 +231,7 @@ func hasColumn(storedTable, expectedTable catalog.TableDescriptor, colName strin func columnExists( storedTable, expectedTable catalog.TableDescriptor, colName string, ) (bool, error) { - _, err := storedTable.FindColumnWithName(tree.Name(colName)) - if err != nil { - if strings.Contains(err.Error(), "does not exist") { - return false, nil - } - return false, err - } - return true, nil + return catalog.FindColumnByName(storedTable, colName) != nil, nil } // columnExistsAndIsNotNull returns true if storedTable contains a non-nullable @@ -254,14 +243,8 @@ func columnExists( func columnExistsAndIsNotNull( storedTable, expectedTable catalog.TableDescriptor, colName string, ) (bool, error) { - storedCol, err := storedTable.FindColumnWithName(tree.Name(colName)) - if err != nil { - if strings.Contains(err.Error(), "does not exist") { - return false, nil - } - return false, err - } - return !storedCol.IsNullable(), nil + storedCol := catalog.FindColumnByName(storedTable, colName) + return storedCol != nil && !storedCol.IsNullable(), nil } // hasIndex returns true if storedTable already has the given index, comparing From fa6333ea2f46ee3b14e2a1469bfe344345b2657c Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Wed, 11 Jan 2023 16:45:56 -0500 Subject: [PATCH 9/9] tabledesc: fix column family validation error message Fixes #95011. Release note: None --- pkg/sql/catalog/tabledesc/validate.go | 7 ++++--- pkg/sql/catalog/tabledesc/validate_test.go | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/sql/catalog/tabledesc/validate.go b/pkg/sql/catalog/tabledesc/validate.go index 49f40c6ae33a..3e668442d632 100644 --- a/pkg/sql/catalog/tabledesc/validate.go +++ b/pkg/sql/catalog/tabledesc/validate.go @@ -1070,13 +1070,14 @@ func (desc *wrapper) validateColumnFamilies(columnsByID map[descpb.ColumnID]cata } for i, colID := range family.ColumnIDs { + colName := family.ColumnNames[i] col, ok := columnsByID[colID] if !ok { - return errors.Newf("family %q contains unknown column \"%d\"", family.Name, colID) + return errors.Newf("family %q contains column reference %q with unknown ID %d", family.Name, colName, colID) } - if col.GetName() != family.ColumnNames[i] { + if col.GetName() != colName { return errors.Newf("family %q column %d should have name %q, but found name %q", - family.Name, colID, col.GetName(), family.ColumnNames[i]) + family.Name, colID, col.GetName(), colName) } if col.IsVirtual() { return errors.Newf("virtual computed column %q cannot be part of a family", col.GetName()) diff --git a/pkg/sql/catalog/tabledesc/validate_test.go b/pkg/sql/catalog/tabledesc/validate_test.go index 285a0b5c3be8..aea282fa2e72 100644 --- a/pkg/sql/catalog/tabledesc/validate_test.go +++ b/pkg/sql/catalog/tabledesc/validate_test.go @@ -589,7 +589,7 @@ func TestValidateTableDesc(t *testing.T) { NextColumnID: 2, NextFamilyID: 1, }}, - {`family "baz" contains unknown column "2"`, + {`family "baz" contains column reference "bar" with unknown ID 2`, descpb.TableDescriptor{ ID: 2, ParentID: 1,