sql: fix drop cascade for nested procedures in legacy schema changer

Release note (bug fix): Fixed a bug that could cause a function
reference to be left behind if a procedure referred to another procedure
that depends on a a table, and that table is dropped with CASCADE.

Author: rafiss

pkg/sql/drop_cascade.go

@@ -243,7 +243,9 @@ func (d *dropCascadeState) dropAllCollectedObjects(ctx context.Context, p *plann
 		if err := p.canDropFunction(ctx, fn); err != nil {
 			return err
 		}
-		if err := p.dropFunctionImpl(ctx, fn); err != nil {
+		// We use DropRestrict here since we've already collected all the functions
+		// in the schema so they will be dropped explicitly.
+		if err := p.dropFunctionImpl(ctx, fn, tree.DropRestrict); err != nil {
 			return err
 		}
 	}

pkg/sql/drop_function.go

@@ -102,7 +102,7 @@ func (p *planner) DropFunction(ctx context.Context, n *tree.DropRoutine) (ret pl
 
 func (n *dropFunctionNode) startExec(params runParams) error {
 	for _, fnMutable := range n.toDrop {
-		if err := params.p.dropFunctionImpl(params.ctx, fnMutable); err != nil {
+		if err := params.p.dropFunctionImpl(params.ctx, fnMutable, n.dropBehavior); err != nil {
 			return err
 		}
 	}
@@ -192,7 +192,9 @@ func (p *planner) canDropFunction(ctx context.Context, fnDesc catalog.FunctionDe
 	return nil
 }
 
-func (p *planner) dropFunctionImpl(ctx context.Context, fnMutable *funcdesc.Mutable) error {
+func (p *planner) dropFunctionImpl(
+	ctx context.Context, fnMutable *funcdesc.Mutable, dropBehavior tree.DropBehavior,
+) error {
 	if fnMutable.Dropped() {
 		return errors.Errorf("function %q is already being dropped", fnMutable.Name)
 	}
@@ -204,6 +206,23 @@ func (p *planner) dropFunctionImpl(ctx context.Context, fnMutable *funcdesc.Muta
 		return scerrors.ConcurrentSchemaChangeError(fnMutable)
 	}
 
+	// Drop dependent functions first if cascade is specified.
+	if dropBehavior == tree.DropCascade {
+		for _, ref := range fnMutable.DependedOnBy {
+			depFuncMutable, err := p.Descriptors().MutableByID(p.txn).Function(ctx, ref.ID)
+			if err != nil {
+				return err
+			}
+			// Skip functions that are already being dropped.
+			if depFuncMutable.Dropped() {
+				continue
+			}
+			if err := p.dropFunctionImpl(ctx, depFuncMutable, dropBehavior); err != nil {
+				return err
+			}
+		}
+	}
+
 	// Remove backreference from tables/views/sequences referenced by this UDF.
 	for _, id := range fnMutable.DependsOn {
 		refMutable, err := p.Descriptors().MutableByID(p.txn).Table(ctx, id)
@@ -310,11 +329,11 @@ func (p *planner) writeDropFuncSchemaChange(ctx context.Context, funcDesc *funcd
 }
 
 func (p *planner) removeDependentFunction(
-	ctx context.Context, tbl *tabledesc.Mutable, fn *funcdesc.Mutable,
+	ctx context.Context, tbl *tabledesc.Mutable, fn *funcdesc.Mutable, dropBehavior tree.DropBehavior,
 ) error {
 	// In the table whose index is being removed, filter out all back-references
 	// that refer to the view that's being removed.
 	tbl.DependedOnBy = removeMatchingReferences(tbl.DependedOnBy, fn.ID)
 	// Then proceed to actually drop the view and log an event for it.
-	return p.dropFunctionImpl(ctx, fn)
+	return p.dropFunctionImpl(ctx, fn, dropBehavior)
 }

pkg/sql/drop_index.go

@@ -547,7 +547,7 @@ func (p *planner) removeDependents(
 			droppedViews = append(droppedViews, cascadedViews...)
 			droppedViews = append(droppedViews, qualifiedView.FQString())
 		case *funcdesc.Mutable:
-			if err := p.removeDependentFunction(ctx, tableDesc, t); err != nil {
+			if err := p.removeDependentFunction(ctx, tableDesc, t, dropBehavior); err != nil {
 				return nil, err
 			}
 		}

pkg/sql/drop_sequence.go

@@ -270,7 +270,7 @@ func dropDependentOnSequence(ctx context.Context, p *planner, seqDesc *tabledesc
 				numDependedOnByTablesToSkip++
 			}
 		case *funcdesc.Mutable:
-			err = p.dropFunctionImpl(ctx, t)
+			err = p.dropFunctionImpl(ctx, t, tree.DropCascade)
 		default:
 			err = errors.AssertionFailedf(
 				"unexpected dependent %s %s on sequence %s",

pkg/sql/drop_table.go

@@ -324,7 +324,7 @@ func (p *planner) dropTableImpl(
 			droppedViews = append(droppedViews, cascadedViews...)
 			droppedViews = append(droppedViews, qualifiedView.FQString())
 		case *funcdesc.Mutable:
-			if err := p.dropFunctionImpl(ctx, t); err != nil {
+			if err := p.dropFunctionImpl(ctx, t, behavior); err != nil {
 				return droppedViews, err
 			}
 		}

pkg/sql/drop_view.go

@@ -313,7 +313,7 @@ func (p *planner) dropViewImpl(
 				cascadeDroppedViews = append(cascadeDroppedViews, cascadedViews...)
 				cascadeDroppedViews = append(cascadeDroppedViews, qualifiedView.FQString())
 			case *funcdesc.Mutable:
-				if err := p.dropFunctionImpl(ctx, t); err != nil {
+				if err := p.dropFunctionImpl(ctx, t, behavior); err != nil {
 					return cascadeDroppedViews, err
 				}
 			}

pkg/sql/logictest/testdata/logic_test/procedure

@@ -554,3 +554,55 @@ CREATE PROCEDURE p142886(p VARCHAR(100)) LANGUAGE SQL AS $$ SELECT 0; $$;
 
 statement ok
 DROP PROCEDURE p142886;
+
+subtest nested_procedure_drop_cascade
+
+statement ok
+CREATE TABLE xy (x INT, y INT);
+
+statement ok
+CREATE PROCEDURE p1_143282() LANGUAGE PLpgSQL AS $$
+  BEGIN
+    INSERT INTO xy VALUES (1, 2) RETURNING x;
+  END;
+$$;
+
+statement ok
+CREATE PROCEDURE p2_143282() LANGUAGE PLpgSQL AS $$
+  DECLARE foo INT;
+  BEGIN
+    CALL p1_143282();
+  END;
+$$;
+
+statement ok
+CREATE PROCEDURE p3_143282() LANGUAGE PLpgSQL AS $$
+  BEGIN
+    CALL p1_143282();
+    CALL p2_143282();
+  END;
+$$;
+
+statement ok
+CALL p3_143282();
+
+# Verify data was inserted twice (once from p1 directly, once via p2)
+query II
+SELECT * FROM xy ORDER BY x
+----
+1  2
+1  2
+
+statement ok
+DROP TABLE xy CASCADE;
+
+statement error procedure p3_143282 does not exist
+CALL p3_143282()
+
+statement error procedure p2_143282 does not exist
+CALL p2_143282()
+
+statement error procedure p1_143282 does not exist
+CALL p1_143282()
+
+subtest end