diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index 97c9a5eb2aae..4559761f5596 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -1615,6 +1615,7 @@ func dropColumnImpl( // You can't drop a column depended on by a view unless CASCADE was // specified. + var depsToDrop catalog.DescriptorIDSet for _, ref := range tableDesc.DependedOnBy { found := false for _, colID := range ref.ColumnIDs { @@ -1632,32 +1633,14 @@ func dropColumnImpl( if err != nil { return nil, err } - depDesc, err := params.p.getDescForCascade( - params.ctx, "column", string(t.Column), tableDesc.ParentID, ref.ID, t.DropBehavior, - ) - if err != nil { - return nil, err - } - switch t := depDesc.(type) { - case *tabledesc.Mutable: - jobDesc := fmt.Sprintf("removing view %q dependent on column %q which is being dropped", - t.Name, colToDrop.ColName()) - cascadedViews, err := params.p.removeDependentView(params.ctx, tableDesc, t, jobDesc) - if err != nil { - return nil, err - } - qualifiedView, err := params.p.getQualifiedTableName(params.ctx, t) - if err != nil { - return nil, err - } + depsToDrop.Add(ref.ID) + } - droppedViews = append(droppedViews, cascadedViews...) - droppedViews = append(droppedViews, qualifiedView.FQString()) - case *funcdesc.Mutable: - if err := params.p.removeDependentFunction(params.ctx, tableDesc, t); err != nil { - return nil, err - } - } + droppedViews, err = params.p.removeDependents( + params.ctx, tableDesc, depsToDrop, "column", colToDrop.GetName(), t.DropBehavior, + ) + if err != nil { + return nil, err } // We cannot remove this column if there are computed columns or a TTL diff --git a/pkg/sql/drop_index.go b/pkg/sql/drop_index.go index 3f7db4ca162e..762ece236627 100644 --- a/pkg/sql/drop_index.go +++ b/pkg/sql/drop_index.go @@ -424,6 +424,7 @@ func (p *planner) dropIndexByName( } var droppedViews []string + var depsToDrop catalog.DescriptorIDSet for _, tableRef := range tableDesc.DependedOnBy { if tableRef.IndexID == idx.GetID() { // Ensure that we have DROP privilege on all dependent views @@ -432,36 +433,17 @@ func (p *planner) dropIndexByName( if err != nil { return err } - depDesc, err := p.getDescForCascade( - ctx, "index", idx.GetName(), tableDesc.ParentID, tableRef.ID, behavior, - ) - if err != nil { - return err - } - switch t := depDesc.(type) { - case *tabledesc.Mutable: - viewJobDesc := fmt.Sprintf("removing view %q dependent on index %q which is being dropped", - t.Name, idx.GetName()) - cascadedViews, err := p.removeDependentView(ctx, tableDesc, t, viewJobDesc) - if err != nil { - return err - } - - qualifiedView, err := p.getQualifiedTableName(ctx, t) - if err != nil { - return err - } - - droppedViews = append(droppedViews, qualifiedView.FQString()) - droppedViews = append(droppedViews, cascadedViews...) - case *funcdesc.Mutable: - if err := p.removeDependentFunction(ctx, tableDesc, t); err != nil { - return err - } - } + depsToDrop.Add(tableRef.ID) } } + droppedViews, err = p.removeDependents( + ctx, tableDesc, depsToDrop, "index", idx.GetName(), behavior, + ) + if err != nil { + return err + } + // Overwriting tableDesc.Index may mess up with the idx object we collected above. Make a copy. idxCopy := *idx.IndexDesc() idxDesc := &idxCopy @@ -533,3 +515,45 @@ func (p *planner) dropIndexByName( CascadeDroppedViews: droppedViews, }) } + +func (p *planner) removeDependents( + ctx context.Context, + tableDesc *tabledesc.Mutable, + depsToDrop catalog.DescriptorIDSet, + typeName string, + objName string, + dropBehavior tree.DropBehavior, +) (droppedViews []string, err error) { + for _, descId := range depsToDrop.Ordered() { + depDesc, err := p.getDescForCascade( + ctx, typeName, objName, tableDesc.ParentID, descId, dropBehavior, + ) + if err != nil { + return nil, err + } + if depDesc.Dropped() { + continue + } + switch t := depDesc.(type) { + case *tabledesc.Mutable: + jobDesc := fmt.Sprintf("removing view %q dependent on %s %q which is being dropped", + t.Name, typeName, objName) + cascadedViews, err := p.removeDependentView(ctx, tableDesc, t, jobDesc) + if err != nil { + return nil, err + } + qualifiedView, err := p.getQualifiedTableName(ctx, t) + if err != nil { + return nil, err + } + + droppedViews = append(droppedViews, cascadedViews...) + droppedViews = append(droppedViews, qualifiedView.FQString()) + case *funcdesc.Mutable: + if err := p.removeDependentFunction(ctx, tableDesc, t); err != nil { + return nil, err + } + } + } + return nil, nil +} diff --git a/pkg/sql/logictest/testdata/logic_test/alter_table b/pkg/sql/logictest/testdata/logic_test/alter_table index 6fd5474b587d..784357d23fd5 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_table +++ b/pkg/sql/logictest/testdata/logic_test/alter_table @@ -3064,3 +3064,74 @@ t_96727_2 CREATE TABLE public.t_96727_2 ( CONSTRAINT t_96727_2_pkey PRIMARY KEY (i ASC), FAMILY fam_0_i_j (i) ) + +subtest regression_97546 + +statement ok +CREATE TABLE t_twocol ( + id INT PRIMARY KEY, + a INT, + b INT, + FAMILY fam_0 (id, a, b) +); +CREATE FUNCTION f1() RETURNS t_twocol AS 'SELECT * FROM t_twocol' LANGUAGE SQL; +CREATE FUNCTION f2() RETURNS t_twocol AS 'SELECT * FROM t_twocol' LANGUAGE SQL; +CREATE FUNCTION f3() RETURNS INT AS 'SELECT a FROM t_twocol' LANGUAGE SQL; + +statement ok +ALTER TABLE t_twocol DROP COLUMN b CASCADE; + +statement error pq: unknown function: f1\(\): function undefined +SELECT f1(); + +statement error pq: unknown function: f2\(\): function undefined +SELECT f2(); + +query T +SELECT create_statement FROM [SHOW CREATE TABLE t_twocol] +---- +CREATE TABLE public.t_twocol ( + id INT8 NOT NULL, + a INT8 NULL, + CONSTRAINT t_twocol_pkey PRIMARY KEY (id ASC), + FAMILY fam_0 (id, a) +) + +statement ok +DROP TABLE t_twocol CASCADE; + +subtest regress_96368 + +statement ok +CREATE TABLE t_twocol ( + id INT PRIMARY KEY, + a INT, + b INT, + FAMILY fam_0 (id, a, b) +); +CREATE FUNCTION f_unqualified_twocol() RETURNS t_twocol AS +$$ + SELECT t_twocol.id, t_twocol.a, t_twocol.b FROM t_twocol; +$$ LANGUAGE SQL; +CREATE FUNCTION f_allcolsel_alias() RETURNS t_twocol AS +$$ + SELECT t1.id, t1.a, t1.b FROM t_twocol AS t1, t_twocol AS t2 WHERE t1.a = t2.a; +$$ LANGUAGE SQL; +CREATE FUNCTION f_tuplestar() RETURNS t_twocol AS +$$ + SELECT t_twocol.id, t_twocol.a, t_twocol.b FROM t_twocol; +$$ LANGUAGE SQL; + +# This statement should be ok but fails +statement ok +ALTER TABLE t_twocol DROP COLUMN b CASCADE; + +query T +SELECT create_statement FROM [SHOW CREATE TABLE t_twocol] +---- +CREATE TABLE public.t_twocol ( + id INT8 NOT NULL, + a INT8 NULL, + CONSTRAINT t_twocol_pkey PRIMARY KEY (id ASC), + FAMILY fam_0 (id, a) +) diff --git a/pkg/sql/schemachanger/scdecomp/decomp.go b/pkg/sql/schemachanger/scdecomp/decomp.go index 064272797e36..ba77e4723976 100644 --- a/pkg/sql/schemachanger/scdecomp/decomp.go +++ b/pkg/sql/schemachanger/scdecomp/decomp.go @@ -19,6 +19,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/scerrors" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" "github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants" "github.com/cockroachdb/cockroach/pkg/sql/sem/catid" @@ -116,6 +117,14 @@ func (w *walkCtx) walkRoot() { case catalog.TableDescriptor: w.walkRelation(d) case catalog.FunctionDescriptor: + if !w.clusterVersion.IsActive(clusterversion.V23_1) { + panic( + scerrors.NotImplementedErrorf( + nil, // n + "function relevant elements and rules are not supported until fully upgraded to 23.1", + ), + ) + } w.walkFunction(d) default: panic(errors.AssertionFailedf("unexpected descriptor type %T: %+v",