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.
cockroachdb · Feb 28, 2023 · 1b7d428 · 1b7d428
1 parent d5a076f
commit 1b7d428
Show file tree

Hide file tree

Showing 4 changed files with 140 additions and 54 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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