From 2e05878b8c600c001c664e78fbe73203febb4ef5 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 ++++++++++++------- .../logictest/testdata/logic_test/alter_table | 71 +++++++++++++++++ pkg/sql/schemachanger/scdecomp/decomp.go | 9 +++ 4 files changed, 139 insertions(+), 52 deletions(-) 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",