Merge #143538 #143574 #143590

143538: sql: fix drop cascade for nested procedures in legacy schema changer r=rafiss a=rafiss

fixes #143282
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.

143574: kvserver: skip TestTxnReadWithinUncertaintyIntervalAfterRangeMerge under duress r=tbg a=miraradeva

This test has always been flaky under deadlock since its introduction. Due to its complexity, we are not going to spend time on improving it now.

Informs: #143563

Release note: None

143590: logictest: deflake zone_config_system_tenant r=rafiss a=rafiss

This assertion was accidentally rewritten in 24ef7d5. Add it back, and add an additional assertion to prevent further accidental rewrites.

fixes #143493
Release note: None

Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Mira Radeva <[email protected]>

Author: 3 people

pkg/kv/kvserver/client_replica_test.go

@@ -930,6 +930,11 @@ func TestTxnReadWithinUncertaintyIntervalAfterLeaseTransfer(t *testing.T) {
 func TestTxnReadWithinUncertaintyIntervalAfterRangeMerge(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
+
+	// This test has always been flaky under deadlock since its introduction. Due
+	// to its complexity, we are not going to spend time on improving it now.
+	skip.UnderDuress(t)
+
 	run := func(t *testing.T, alignLeaseholders bool, alsoSplit bool) {
 
 		// The stores 0 and 1 are the "LHS", and the stores 2 and 3 are the RHS.

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

pkg/sql/logictest/testdata/logic_test/zone_config_system_tenant

@@ -141,6 +141,18 @@ FROM system.span_configurations
 WHERE end_key > (SELECT crdb_internal.table_span($t_id)[1])
 ORDER BY start_key
 ----
+/Table/110  {"gcPolicy": {"ttlSeconds": 90001}, "numReplicas": 3, "rangeMaxBytes": "67108864", "rangeMinBytes": "1048576"}
+/Table/111  {"gcPolicy": {"ttlSeconds": 90001}, "numReplicas": 3, "rangeMaxBytes": "1073741824", "rangeMinBytes": "67108864"}
+
+# Run the same query again and make sure there are 2 rows. This assertion is
+# only here to prevent the previous test case from being rewritten accidentally.
+statement count 2
+SELECT
+  crdb_internal.pretty_key(start_key, -1),
+  crdb_internal.pb_to_json('cockroach.roachpb.SpanConfig', config)
+FROM system.span_configurations
+WHERE end_key > (SELECT crdb_internal.table_span($t_id)[1])
+ORDER BY start_key
 
 subtest transactional_schemachanges