From b2d689e5ad04191104a758e685abb0d097a238bf Mon Sep 17 00:00:00 2001 From: Chengxiong Ruan Date: Fri, 24 Feb 2023 11:39:06 -0500 Subject: [PATCH] sql: create new slice when removing backreference items Fixes: #97546 Fixes: #96368 We are reusing the underlying slots of the `DependedOnBy` slice when removing items from it. This is problematic because we normally have to iterate the same slice to figure out what to drop. Iterating on a slice which is rewritten can be problematic unless we always know how to smartly move the cursor backward. This commit remove such kind of hazard by collecting descriptor IDs to drop before actually dropping everything. Releaste note (sql change): This commit fixes two bugs prevent users from doing `DROP COLUMN CASCADE` and `DROP INDEX CASCADE` when dependecies on the Table from Views and Functions get complex. They all happened When a table is referenced multiple times in more than one Views and Functions. Release note: None. --- pkg/sql/alter_table.go | 33 ++------ pkg/sql/drop_index.go | 78 ++++++++++++------- pkg/sql/drop_table.go | 2 +- .../logictest/testdata/logic_test/alter_table | 71 +++++++++++++++++ pkg/sql/schemachanger/scdecomp/decomp.go | 9 +++ 5 files changed, 140 insertions(+), 53 deletions(-) diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index ec2072d5a4f2..c3c1d6018794 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -1608,6 +1608,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 { @@ -1625,32 +1626,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/drop_table.go b/pkg/sql/drop_table.go index 59c1464134d8..56481facc672 100644 --- a/pkg/sql/drop_table.go +++ b/pkg/sql/drop_table.go @@ -663,7 +663,7 @@ func removeFKBackReferenceFromTable( func removeMatchingReferences( refs []descpb.TableDescriptor_Reference, id descpb.ID, ) []descpb.TableDescriptor_Reference { - updatedRefs := refs[:0] + var updatedRefs []descpb.TableDescriptor_Reference for _, ref := range refs { if ref.ID != id { updatedRefs = append(updatedRefs, ref) diff --git a/pkg/sql/logictest/testdata/logic_test/alter_table b/pkg/sql/logictest/testdata/logic_test/alter_table index 6c87228e43ae..ec16a7542804 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_table +++ b/pkg/sql/logictest/testdata/logic_test/alter_table @@ -2940,3 +2940,74 @@ ALTER TABLE t_96648 DROP CONSTRAINT check_i, DROP CONSTRAINT check_i1; statement ok DROP TABLE t_96648 + +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..8e187e7c6bad 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, + "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",