From 52385dc8e71fc1f45953aa5fa758d8c3ffc804a3 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Thu, 9 Jul 2026 18:22:05 +0900 Subject: [PATCH v2 2/3] 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 Discussion: https://postgr.es/m/20260705222115.be.noahmisch@microsoft.com Backpatch-through: 19 --- src/backend/commands/trigger.c | 29 ++++++++++++++-- src/test/regress/expected/foreign_key.out | 41 +++++++++++++++++++++++ src/test/regress/sql/foreign_key.sql | 39 +++++++++++++++++++++ 3 files changed, 107 insertions(+), 2 deletions(-) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index ce39ce2e9a2..39a1576e75d 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -3951,6 +3951,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 */ }; @@ -5521,6 +5523,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; } @@ -5621,8 +5626,28 @@ 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; } /* diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index db2dbf59899..af8e4a09b9c 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -3979,3 +3979,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; diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index 3485012c386..0f8386f89fd 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -2912,3 +2912,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; -- 2.47.3