From fe4b409a3c0b3f09d9b0034302bef6127e1cc9c0 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Wed, 19 Aug 2026 21:58:48 +0900 Subject: [PATCH v3 1/3] Restore firing state at subtransaction end AfterTriggerEndQuery(), AfterTriggerFireDeferred(), and AfterTriggerSetState() bracket their firing loops with firing_depth++/--. The decrement runs after the loop and is not protected by PG_FINALLY, so an error caught by a subtransaction (e.g. a PL/pgSQL EXCEPTION block) leaves firing_depth too high. Separately, AfterTriggerEndSubXact() unconditionally cleared firing_batch_callbacks, even if the subtransaction began while an outer batch-callback loop was active. firing_depth feeds AfterTriggerIsActive(), which the RI fast path uses to decide whether an FK check is running inside trigger firing and may batch. A stranded firing_depth 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. The check is routed into the batched fast path, but a utility command has no AfterTriggerEndQuery() to fire the flush callback. The violating row is not reported, the constraint is marked validated, and the cached PK relation and index leak. Save firing_depth and firing_batch_callbacks at subtransaction start and restore them in AfterTriggerEndSubXact(), next to the existing query_depth handling. Restoring, rather than zeroing or 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 may be legitimately set. 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 | 40 ++++++++++++++++++++++ 3 files changed, 108 insertions(+), 2 deletions(-) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index b7881bf4a29..11941b5f5c3 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 b699164260a..01343c58e11 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -3910,3 +3910,44 @@ DROP TYPE fkint CASCADE; NOTICE: drop cascades to 2 other objects DETAIL: drop cascades to function fkint_in(cstring) drop cascades to function fkint_out(fkint) +-- 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 31736251d78..987cea61ba2 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -2857,3 +2857,43 @@ DROP TABLE pktable_inval; DROP CAST (fkint AS int4); DROP FUNCTION fkint_to_int4(fkint); DROP TYPE fkint CASCADE; + +-- 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