Restore firing state at subtransaction end

AfterTriggerEndQuery(), AfterTriggerFireDeferred(), and
AfterTriggerSetState() bracket their firing loops with
firing_depth++/--, and FireAfterTriggerBatchCallbacks() brackets its
loop with firing_batch_callbacks.  The closing step runs after the loop
and is not protected by PG_FINALLY, so if a trigger or batch callback
throws and the error is caught by a subtransaction (e.g. a PL/pgSQL
EXCEPTION block), firing_depth is left too high and/or
firing_batch_callbacks is left set for the rest of the transaction.

firing_depth feeds AfterTriggerIsActive(), which the RI fast path uses
to decide whether an FK check is running inside trigger firing (and
may batch).  firing_depth left too high makes AfterTriggerIsActive()
wrongly report firing as active afterwards.  This is reachable and
results in silent data corruption: after a caught FK-check error, an
ALTER TABLE ... ADD FOREIGN KEY whose validation runs per-row
(RI_Initial_Check() having bailed, e.g. because RLS is enabled on the
referenced table) calls RI_FKey_check() with AfterTriggerIsActive()
wrongly true, so the check is routed into the batched fast path.  A
utility command has no AfterTriggerEndQuery() to fire the flush
callback, so the batch is never flushed: the violating row is not
reported, the constraint is marked validated, and the cached PK
relation and index leak.

Fix by restoring firing_depth and firing_batch_callbacks in
AfterTriggerEndSubXact() to the values saved at subtransaction start,
next to the existing query_depth handling.  Restoring (rather than
zeroing/clearing) is required because a subtransaction can begin and
end while an outer query is firing, where firing_depth is legitimately
positive and firing_batch_callbacks legitimately set; forcing them to
0/false there breaks the outer firing (FireAfterTriggerBatchCallbacks()
asserts firing_depth > 0).

Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260705222115.be.noahmisch@microsoft.com
Backpatch-through: 19

Author: amitlan

src/backend/commands/trigger.c

@@ -3946,6 +3946,8 @@ struct AfterTriggersTransData
 	SetConstraintState state;	/* saved S C state, or NULL if not yet saved */
 	AfterTriggerEventList events;	/* saved list pointer */
 	int			query_depth;	/* saved query_depth */
+	int			firing_depth;	/* saved firing_depth */
+	bool		firing_batch_callbacks; /* saved firing_batch_callbacks */
 	CommandId	firing_counter; /* saved firing_counter */
 };
 
@@ -5507,6 +5509,9 @@ AfterTriggerBeginSubXact(void)
 	afterTriggers.trans_stack[my_level].state = NULL;
 	afterTriggers.trans_stack[my_level].events = afterTriggers.events;
 	afterTriggers.trans_stack[my_level].query_depth = afterTriggers.query_depth;
+	afterTriggers.trans_stack[my_level].firing_depth = afterTriggers.firing_depth;
+	afterTriggers.trans_stack[my_level].firing_batch_callbacks =
+		afterTriggers.firing_batch_callbacks;
 	afterTriggers.trans_stack[my_level].firing_counter = afterTriggers.firing_counter;
 }
 
@@ -5607,8 +5612,27 @@ AfterTriggerEndSubXact(bool isCommit)
 		}
 	}
 
-	/* Reset in case a callback threw an error while firing. */
-	afterTriggers.firing_batch_callbacks = false;
+	/*
+	 * Restore firing_depth and firing_batch_callbacks to their values at
+	 * subtransaction start.  The matching decrement of firing_depth in
+	 * AfterTriggerEndQuery()/AfterTriggerFireDeferred(), and the clearing of
+	 * firing_batch_callbacks in FireAfterTriggerBatchCallbacks(), run after
+	 * their loops and are not protected by PG_FINALLY.  A trigger or batch
+	 * callback error caught by this subtransaction can therefore leave either
+	 * one set; restoring the saved values unwinds only this subtransaction's
+	 * firing.
+	 *
+	 * Restoring (rather than zeroing/clearing) matters because a subtransaction
+	 * can begin and end while an outer query's triggers are firing -- for
+	 * instance a batch callback whose user-supplied cast or equality function
+	 * runs DML in a BEGIN ... EXCEPTION block.  There firing_depth is positive
+	 * and firing_batch_callbacks is true; forcing them to 0/false would corrupt
+	 * the outer firing (FireAfterTriggerBatchCallbacks() asserts firing_depth
+	 * > 0, and clearing the guard would defeat its re-entrancy check).
+	 */
+	afterTriggers.firing_depth = afterTriggers.trans_stack[my_level].firing_depth;
+	afterTriggers.firing_batch_callbacks =
+		afterTriggers.trans_stack[my_level].firing_batch_callbacks;
 }
 
 /*

src/test/regress/expected/foreign_key.out

@@ -3864,3 +3864,44 @@ SELECT count(*) AS deferred_rows FROM fp_deferred_fk;  -- 1, check passed at com
 (1 row)
 
 DROP TABLE fp_deferred_fk, fp_deferred_pk;
+-- Stranded firing state must not misroute ALTER TABLE ... ADD FOREIGN KEY
+-- validation into the batched fast path.  A caught FK-check error inside a
+-- subtransaction leaves firing_depth set (its decrement is skipped); a
+-- following ALTER whose validation runs per-row (forced here by RLS on the
+-- referenced table, so RI_Initial_Check() bails) would then be wrongly treated
+-- as running inside trigger firing, batched, and never flushed (a utility
+-- command has no AfterTriggerEndQuery), silently validating a violating row.
+CREATE ROLE regress_fpav_role;
+CREATE TABLE fpav_pk (id int PRIMARY KEY);
+INSERT INTO fpav_pk VALUES (1);
+ALTER TABLE fpav_pk ENABLE ROW LEVEL SECURITY;
+CREATE POLICY fpav_pk_all ON fpav_pk FOR ALL USING (true) WITH CHECK (true);
+GRANT REFERENCES, SELECT ON fpav_pk TO regress_fpav_role;
+CREATE TABLE fpav_fk (a int);
+INSERT INTO fpav_fk VALUES (1), (99);
+ALTER TABLE fpav_fk OWNER TO regress_fpav_role;
+CREATE TABLE fpav_cv_pk (id int PRIMARY KEY);
+INSERT INTO fpav_cv_pk VALUES (1);
+CREATE TABLE fpav_cv_fk (a int REFERENCES fpav_cv_pk(id));
+GRANT INSERT ON fpav_cv_fk TO regress_fpav_role;
+GRANT SELECT, INSERT ON fpav_cv_pk TO regress_fpav_role;
+SET ROLE regress_fpav_role;
+BEGIN;
+-- Caught FK violation: leaves firing_depth set if it is not restored.
+DO $$
+BEGIN
+    BEGIN
+        INSERT INTO fpav_cv_fk VALUES (999);
+    EXCEPTION WHEN foreign_key_violation THEN
+        NULL;
+    END;
+END$$;
+-- Must ERROR on the violating row (99), not silently validate it.
+ALTER TABLE fpav_fk ADD CONSTRAINT fpav_fk_fkey
+    FOREIGN KEY (a) REFERENCES fpav_pk (id);
+ERROR:  insert or update on table "fpav_fk" violates foreign key constraint "fpav_fk_fkey"
+DETAIL:  Key (a)=(99) is not present in table "fpav_pk".
+ROLLBACK;
+RESET ROLE;
+DROP TABLE fpav_fk, fpav_pk, fpav_cv_fk, fpav_cv_pk;
+DROP ROLE regress_fpav_role;

src/test/regress/sql/foreign_key.sql

@@ -2807,3 +2807,42 @@ INSERT INTO fp_deferred_pk VALUES (1);
 COMMIT;
 SELECT count(*) AS deferred_rows FROM fp_deferred_fk;  -- 1, check passed at commit
 DROP TABLE fp_deferred_fk, fp_deferred_pk;
+-- Stranded firing state must not misroute ALTER TABLE ... ADD FOREIGN KEY
+-- validation into the batched fast path.  A caught FK-check error inside a
+-- subtransaction leaves firing_depth set (its decrement is skipped); a
+-- following ALTER whose validation runs per-row (forced here by RLS on the
+-- referenced table, so RI_Initial_Check() bails) would then be wrongly treated
+-- as running inside trigger firing, batched, and never flushed (a utility
+-- command has no AfterTriggerEndQuery), silently validating a violating row.
+CREATE ROLE regress_fpav_role;
+CREATE TABLE fpav_pk (id int PRIMARY KEY);
+INSERT INTO fpav_pk VALUES (1);
+ALTER TABLE fpav_pk ENABLE ROW LEVEL SECURITY;
+CREATE POLICY fpav_pk_all ON fpav_pk FOR ALL USING (true) WITH CHECK (true);
+GRANT REFERENCES, SELECT ON fpav_pk TO regress_fpav_role;
+CREATE TABLE fpav_fk (a int);
+INSERT INTO fpav_fk VALUES (1), (99);
+ALTER TABLE fpav_fk OWNER TO regress_fpav_role;
+CREATE TABLE fpav_cv_pk (id int PRIMARY KEY);
+INSERT INTO fpav_cv_pk VALUES (1);
+CREATE TABLE fpav_cv_fk (a int REFERENCES fpav_cv_pk(id));
+GRANT INSERT ON fpav_cv_fk TO regress_fpav_role;
+GRANT SELECT, INSERT ON fpav_cv_pk TO regress_fpav_role;
+SET ROLE regress_fpav_role;
+BEGIN;
+-- Caught FK violation: leaves firing_depth set if it is not restored.
+DO $$
+BEGIN
+    BEGIN
+        INSERT INTO fpav_cv_fk VALUES (999);
+    EXCEPTION WHEN foreign_key_violation THEN
+        NULL;
+    END;
+END$$;
+-- Must ERROR on the violating row (99), not silently validate it.
+ALTER TABLE fpav_fk ADD CONSTRAINT fpav_fk_fkey
+    FOREIGN KEY (a) REFERENCES fpav_pk (id);
+ROLLBACK;
+RESET ROLE;
+DROP TABLE fpav_fk, fpav_pk, fpav_cv_fk, fpav_cv_pk;
+DROP ROLE regress_fpav_role;