From 1b7d4288d82f59377856bb9d91e58f45bcca3ff8 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 | 34 ++------ pkg/sql/drop_index.go | 78 +++++++++++------- .../logictest/testdata/logic_test/alter_table | 80 +++++++++++++++++++ pkg/sql/pgwire/testdata/pgtest/notice | 2 +- 4 files changed, 140 insertions(+), 54 deletions(-) diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index 79b26c050edb..5e72c602da1d 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -29,7 +29,6 @@ import ( "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/funcdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver" "github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" @@ -1636,6 +1635,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 { @@ -1653,32 +1653,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 fc7a1e338441..55418e720ced 100644 --- a/pkg/sql/drop_index.go +++ b/pkg/sql/drop_index.go @@ -444,6 +444,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 @@ -452,36 +453,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 @@ -560,3 +542,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 droppedViews, nil +} diff --git a/pkg/sql/logictest/testdata/logic_test/alter_table b/pkg/sql/logictest/testdata/logic_test/alter_table index 389be7f3b8ff..f020ea97fd88 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_table +++ b/pkg/sql/logictest/testdata/logic_test/alter_table @@ -2748,3 +2748,83 @@ query IT SELECT c1, c3 FROM t_93398; ---- 0 f + +subtest regression_97546 + +skipif config local-mixed-22.1-22.2 +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 id, a, b FROM t_twocol' LANGUAGE SQL; +CREATE FUNCTION f2() RETURNS t_twocol AS 'SELECT id, a, b FROM t_twocol' LANGUAGE SQL; +CREATE FUNCTION f3() RETURNS INT AS 'SELECT a FROM t_twocol' LANGUAGE SQL; + +skipif config local-mixed-22.1-22.2 +statement ok +ALTER TABLE t_twocol DROP COLUMN b CASCADE; + +skipif config local-mixed-22.1-22.2 +statement error pq: unknown function: f1\(\): function undefined +SELECT f1(); + +skipif config local-mixed-22.1-22.2 +statement error pq: unknown function: f2\(\): function undefined +SELECT f2(); + +skipif config local-mixed-22.1-22.2 +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) +) + +skipif config local-mixed-22.1-22.2 +statement ok +DROP TABLE t_twocol CASCADE; + +subtest regress_96368 + +skipif config local-mixed-22.1-22.2 +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 +skipif config local-mixed-22.1-22.2 +statement ok +ALTER TABLE t_twocol DROP COLUMN b CASCADE; + +skipif config local-mixed-22.1-22.2 +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/pgwire/testdata/pgtest/notice b/pkg/sql/pgwire/testdata/pgtest/notice index 1193cf6e078e..f1906b9de6f6 100644 --- a/pkg/sql/pgwire/testdata/pgtest/notice +++ b/pkg/sql/pgwire/testdata/pgtest/notice @@ -55,7 +55,7 @@ Query {"String": "DROP INDEX t_x_idx"} until crdb_only CommandComplete ---- -{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"the data for dropped indexes is reclaimed asynchronously","Detail":"","Hint":"The reclamation delay can be customized in the zone configuration for the table.","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"drop_index.go","Line":547,"Routine":"dropIndexByName","UnknownFields":null} +{"Severity":"NOTICE","SeverityUnlocalized":"NOTICE","Code":"00000","Message":"the data for dropped indexes is reclaimed asynchronously","Detail":"","Hint":"The reclamation delay can be customized in the zone configuration for the table.","Position":0,"InternalPosition":0,"InternalQuery":"","Where":"","SchemaName":"","TableName":"","ColumnName":"","DataTypeName":"","ConstraintName":"","File":"drop_index.go","Line":529,"Routine":"dropIndexByName","UnknownFields":null} {"Type":"CommandComplete","CommandTag":"DROP INDEX"} until noncrdb_only