Merge #97631

97631: sql: create new slice when removing backreference items r=chengxiong-ruan a=chengxiong-ruan Fixes: #97546 Fixes: #96368 Previously we reused 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 changes it to just create a new slice to avoid the hazzard. Release note: None. Co-authored-by: Chengxiong Ruan <[email protected]>
cockroachdb · Feb 28, 2023 · 46b1afa · 46b1afa
2 parents b0e5507 + 2e05878
commit 46b1afa
Show file tree

Hide file tree

Showing 4 changed files with 139 additions and 52 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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",