Merge #148107

148107: sql: prevent backref leak on self-referencing triggers r=spilchen a=spilchen

Prior to this change, dropping a trigger on a table that had a self-referencing foreign key could leave a dangling back-reference to the table itself. This incorrect dependency caused DROP TABLE/SCHEMA/DATABASE to loop indefinitely.

During depedency cleanup of DROP TRIGGER, we omit FKs when collecting the forward references. FKs can safely be omitted because they aren't tracked in the DependedOnBy relationship. They have thier own separate tracking in the table descriptor.

Fixes #147981

Epic: none
Release note: none

Note: omitting a bug-fix release note because this is a problem only on master.

Co-authored-by: Matt Spilchen <[email protected]>

Author: craig[bot]

pkg/ccl/logictestccl/testdata/logic_test/triggers

@@ -4696,6 +4696,67 @@ DROP FUNCTION f1;
 
 subtest end
 
+subtest self_ref_and_fk
+
+statement ok
+CREATE SCHEMA sc1;
+
+statement ok
+CREATE TABLE sc1.tab1 (
+    col1 INT,
+    PRIMARY KEY (col1)
+);
+
+# Self-referencing FK
+statement ok
+ALTER TABLE sc1.tab1
+ADD CONSTRAINT fk_self FOREIGN KEY (col1)
+REFERENCES sc1.tab1 (col1)
+ON DELETE CASCADE;
+
+# Trigger function that selects from same table
+statement ok
+CREATE FUNCTION tf1() RETURNS TRIGGER AS $$
+BEGIN
+  SELECT 1 FROM sc1.tab1;
+  RETURN NULL;
+END;
+$$ LANGUAGE PLpgSQL;
+
+# Create the trigger that adds a forward and backward reference on sc1.tab1.
+statement ok
+CREATE TRIGGER tr1
+BEFORE INSERT OR UPDATE ON sc1.tab1
+FOR EACH ROW EXECUTE FUNCTION tf1();
+
+# Drop the trigger. Prior to #147981, this used to leave the backref to sc1.tab1
+# around causing an infinite loop during DROP SCHEMA.
+statement ok
+DROP TRIGGER tr1 ON sc1.tab1;
+
+# This test doesn't run with the legacy schema config because CREATE/DROP
+# trigger isn't implemented there. We enable legacy to run DROP DDL to verify
+# the dependencies handling. We do that in a transaction that is rolled back so
+# we can retry with the DSC.
+let $use_decl_sc
+SHOW use_declarative_schema_changer
+
+statement ok
+set use_declarative_schema_changer = 'off';
+
+statement ok
+BEGIN;
+DROP SCHEMA sc1 CASCADE;
+ROLLBACK;
+
+statement ok
+set use_declarative_schema_changer = $use_decl_sc;
+
+statement ok
+DROP SCHEMA sc1 CASCADE;
+
+subtest end
+
 # ==============================================================================
 # Test information_schema.triggers and pg_catalog.pg_trigger
 # ==============================================================================

pkg/sql/catalog/descriptor.go

@@ -641,10 +641,10 @@ type TableDescriptor interface {
 	// is now deprecated.
 	GetReplacementOf() descpb.TableDescriptor_Replacement
 
-	// GetAllReferencedTableIDs returns all relation IDs that this table
-	// references. Table references can be from foreign keys, triggers, or via
-	// direct references if the descriptor is a view.
-	GetAllReferencedTableIDs() descpb.IDs
+	// GetAllReferencedRelationIDsExceptFKs returns the IDs of all relations
+	// this table depends on, excluding foreign key dependencies. Dependencies can
+	// originate from triggers, policies, or direct references in views.
+	GetAllReferencedRelationIDsExceptFKs() descpb.IDs
 
 	// GetAllReferencedTypeIDs returns all user defined type descriptor IDs that
 	// this table references. It takes in a function that returns the TypeDescriptor

pkg/sql/catalog/tabledesc/structured.go

@@ -375,17 +375,10 @@ func ForEachExprStringInTableDesc(
 	return nil
 }
 
-// GetAllReferencedTableIDs implements the TableDescriptor interface.
-func (desc *wrapper) GetAllReferencedTableIDs() descpb.IDs {
+// GetAllReferencedRelationIDsExceptFKs implements the TableDescriptor interface.
+func (desc *wrapper) GetAllReferencedRelationIDsExceptFKs() descpb.IDs {
 	var ids catalog.DescriptorIDSet
 
-	// Collect referenced table IDs in foreign keys.
-	for _, fk := range desc.OutboundForeignKeys() {
-		ids.Add(fk.GetReferencedTableID())
-	}
-	for _, fk := range desc.InboundForeignKeys() {
-		ids.Add(fk.GetOriginTableID())
-	}
 	// Add trigger dependencies.
 	for i := range desc.Triggers {
 		ids = ids.Union(catalog.MakeDescriptorIDSet(desc.Triggers[i].DependsOn...))

pkg/sql/schemachanger/scexec/scmutationexec/references.go

@@ -689,7 +689,9 @@ func (i *immediateVisitor) UpdateTableBackReferencesInRelations(
 	if err != nil {
 		return err
 	}
-	forwardRefs := backRefTbl.GetAllReferencedTableIDs()
+	// Collect all forward references from this table, excluding foreign keys.
+	// Foreign key dependencies are not tracked via DependedOnBy, so we skip them here.
+	forwardRefs := backRefTbl.GetAllReferencedRelationIDsExceptFKs()
 	for _, ref := range op.RelationReferences {
 		referenced, err := i.checkOutTable(ctx, ref.ID)
 		if err != nil {

pkg/workload/schemachange/optype.go

@@ -315,7 +315,7 @@ var opWeights = []int{
 	dropSchema:                        1,
 	dropSequence:                      1,
 	dropTable:                         1,
-	dropTrigger:                       0, // TODO(147981): re-enable once we fix orphaning of backref in DROP TRIGGER
+	dropTrigger:                       1,
 	dropView:                          1,
 	renameIndex:                       1,
 	renameSequence:                    1,