From 54220a5334ec94e1915bd014abbd93db0866d558 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Mon, 31 Aug 2026 17:20:26 +0900 Subject: [PATCH v2 1/2] Don't let ALTER TABLE validation join a trigger's RI batch RI fast-path batching defers a foreign key probe to the end of the trigger-firing cycle that queued the check. RI_FKey_check() decided whether it could batch by asking AfterTriggerIsActive(), that is, whether any firing cycle was in progress at all. validateForeignKeyConstraint() calls the insert-check function directly for each existing row, and no cycle queues those checks. AfterTriggerIsActive() was taken to distinguish that case, on the assumption that validation runs outside any firing cycle. ALTER TABLE ... ADD FOREIGN KEY can itself run from an AFTER trigger, and then it does not: a cycle is in progress, one that has nothing to do with validation, and the validation rows joined its batch. ALTER TABLE marked the constraint validated as soon as the scan finished, before anything flushed them. If the trigger had opened a subtransaction, as a PL/pgSQL BEGIN ... EXCEPTION block does, its commit discarded the batch outright, leaving a validated constraint over a table still containing an orphan row. Assert-enabled builds crashed instead. Ambient after-trigger state cannot answer the question: validation and a genuine RI trigger reach RI_FKey_check() with identical after-trigger context. Only the caller knows whether a flush is guaranteed to run before the result is observed. Pass it in instead -- give RI_FKey_check() an allow_batch argument and add RI_FKey_check_validate() for the validation scan to call -- and remove AfterTriggerIsActive(), which has no remaining callers. Reported-by: Ayush Tiwari Discussion: https://postgr.es/m/CAJTYsWU-cihumKaCtnCuV=iuRVcha=3vHa9eXA_vA=cgD2w+Ew@mail.gmail.com Backpatch-through: 19 --- src/backend/commands/tablecmds.c | 12 +----- src/backend/commands/trigger.c | 14 ------ src/backend/utils/adt/ri_triggers.c | 25 +++++++++-- src/include/commands/trigger.h | 2 +- src/test/regress/expected/foreign_key.out | 52 +++++++++++++++++++++++ src/test/regress/sql/foreign_key.sql | 44 +++++++++++++++++++ 6 files changed, 119 insertions(+), 30 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index fd144d783d9..0180c363bc7 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -14233,18 +14233,10 @@ validateForeignKeyConstraint(char *conname, while (table_scan_getnextslot(scan, ForwardScanDirection, slot)) { - LOCAL_FCINFO(fcinfo, 0); TriggerData trigdata = {0}; CHECK_FOR_INTERRUPTS(); - /* - * Make a call to the trigger function - * - * No parameters are passed, but we do set a context - */ - MemSet(fcinfo, 0, SizeForFunctionCallInfo(0)); - /* * We assume RI_FKey_check_ins won't look at flinfo... */ @@ -14255,9 +14247,7 @@ validateForeignKeyConstraint(char *conname, trigdata.tg_trigslot = slot; trigdata.tg_trigger = &trig; - fcinfo->context = (Node *) &trigdata; - - RI_FKey_check_ins(fcinfo); + RI_FKey_check_validate(&trigdata); MemoryContextReset(perTupCxt); } diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 79ffd2cada6..d6eda9ffb2d 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -6961,20 +6961,6 @@ FireAfterTriggerBatchCallbacks(List *callbacks) afterTriggers.firing_batch_callbacks = false; } -/* - * AfterTriggerIsActive - * Returns true if we're inside the after-trigger framework where - * registered batch callbacks will actually be invoked. - * - * This is false during validateForeignKeyConstraint(), which calls - * RI trigger functions directly outside the after-trigger framework. - */ -bool -AfterTriggerIsActive(void) -{ - return afterTriggers.firing_depth > 0; -} - /* * AfterTriggerCurrentQueryDepth * Return the current after-trigger query nesting depth. diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index d4545618634..1e69e7d93d2 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -412,7 +412,7 @@ static void ri_FastPathTeardown(int depth); * Check foreign key existence (combined for INSERT and UPDATE). */ static Datum -RI_FKey_check(TriggerData *trigdata) +RI_FKey_check(TriggerData *trigdata, bool allow_batch) { RI_ConstraintInfo *riinfo; Relation fk_rel; @@ -520,7 +520,7 @@ RI_FKey_check(TriggerData *trigdata) */ if (ri_fastpath_is_applicable(riinfo)) { - if (AfterTriggerIsActive() && !ri_fastpath_flushing) + if (allow_batch && !ri_fastpath_flushing) { /* Batched path: buffer and probe in groups */ ri_FastPathBatchAdd(riinfo, fk_rel, newslot); @@ -675,7 +675,7 @@ RI_FKey_check_ins(PG_FUNCTION_ARGS) ri_CheckTrigger(fcinfo, "RI_FKey_check_ins", RI_TRIGTYPE_INSERT); /* Share code with UPDATE case. */ - return RI_FKey_check((TriggerData *) fcinfo->context); + return RI_FKey_check((TriggerData *) fcinfo->context, true); } @@ -691,7 +691,7 @@ RI_FKey_check_upd(PG_FUNCTION_ARGS) ri_CheckTrigger(fcinfo, "RI_FKey_check_upd", RI_TRIGTYPE_UPDATE); /* Share code with INSERT case. */ - return RI_FKey_check((TriggerData *) fcinfo->context); + return RI_FKey_check((TriggerData *) fcinfo->context, true); } @@ -2001,6 +2001,23 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel) return true; } +/* + * RI_FKey_check_validate - + * + * Check one existing row during ALTER TABLE ... ADD FOREIGN KEY validation. + * + * This is the check the insert trigger performs, but never batched. Batching + * defers the probe to the end of the firing cycle that queued the check; + * validation is queued by no cycle, and ALTER TABLE marks the constraint + * validated as soon as the scan completes, so a deferred probe would be + * reported -- or discarded -- after the result is already visible. + */ +void +RI_FKey_check_validate(TriggerData *trigdata) +{ + (void) RI_FKey_check(trigdata, false); +} + /* * RI_PartitionRemove_Check - * diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h index fecdb785f35..6967653ad5c 100644 --- a/src/include/commands/trigger.h +++ b/src/include/commands/trigger.h @@ -279,6 +279,7 @@ extern bool RI_FKey_fk_upd_check_required(Trigger *trigger, Relation fk_rel, TupleTableSlot *oldslot, TupleTableSlot *newslot); extern bool RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel); +extern void RI_FKey_check_validate(TriggerData *trigdata); extern void RI_PartitionRemove_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel); @@ -308,7 +309,6 @@ typedef void (*AfterTriggerBatchCallback) (void *arg); extern void RegisterAfterTriggerBatchCallback(AfterTriggerBatchCallback callback, void *arg); -extern bool AfterTriggerIsActive(void); extern int AfterTriggerCurrentQueryDepth(void); extern void AtEOXact_RI(bool isCommit); diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index ac044eb40fa..b7bf5b9fa01 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -4113,3 +4113,55 @@ SELECT count(*) AS deferred_rows FROM fp_deferred_fk; -- 1, check passed at com (1 row) DROP TABLE fp_deferred_fk, fp_deferred_pk; +-- ALTER TABLE ... ADD FOREIGN KEY run from inside an AFTER trigger. The +-- validation scan must check each row itself rather than batching, so the +-- orphan row is caught, the ALTER fails, and the constraint is never created. +-- +-- Row-level security on the referenced table forces the row-at-a-time path; +-- RI_Initial_Check() cannot use its single query when RLS applies. +CREATE ROLE regress_fp_alter_role; +CREATE TABLE fp_alter_pk (id int PRIMARY KEY); +INSERT INTO fp_alter_pk VALUES (1); +ALTER TABLE fp_alter_pk ENABLE ROW LEVEL SECURITY; +CREATE POLICY fp_alter_pk_all ON fp_alter_pk USING (true); +GRANT REFERENCES, SELECT ON fp_alter_pk TO regress_fp_alter_role; +CREATE TABLE fp_alter_fk (a int); +INSERT INTO fp_alter_fk VALUES (1), (999); +ALTER TABLE fp_alter_fk OWNER TO regress_fp_alter_role; +CREATE TABLE fp_alter_outer (a int); +ALTER TABLE fp_alter_outer OWNER TO regress_fp_alter_role; +CREATE FUNCTION fp_alter_from_trigger() RETURNS trigger +LANGUAGE plpgsql AS $$ +BEGIN + BEGIN + EXECUTE 'ALTER TABLE fp_alter_fk ADD CONSTRAINT fp_alter_bad_fk ' + 'FOREIGN KEY (a) REFERENCES fp_alter_pk(id)'; + EXCEPTION WHEN others THEN + RAISE; + END; + RETURN NEW; +END$$; +CREATE TRIGGER fp_alter_trg AFTER INSERT ON fp_alter_outer + FOR EACH ROW EXECUTE FUNCTION fp_alter_from_trigger(); +SET ROLE regress_fp_alter_role; +INSERT INTO fp_alter_outer VALUES (1); +ERROR: insert or update on table "fp_alter_fk" violates foreign key constraint "fp_alter_bad_fk" +DETAIL: Key (a)=(999) is not present in table "fp_alter_pk". +CONTEXT: SQL statement "ALTER TABLE fp_alter_fk ADD CONSTRAINT fp_alter_bad_fk FOREIGN KEY (a) REFERENCES fp_alter_pk(id)" +PL/pgSQL function fp_alter_from_trigger() line 4 at EXECUTE +RESET ROLE; +SELECT conname, convalidated FROM pg_constraint WHERE conname = 'fp_alter_bad_fk'; + conname | convalidated +---------+-------------- +(0 rows) + +TABLE fp_alter_fk; + a +----- + 1 + 999 +(2 rows) + +DROP TABLE fp_alter_outer, fp_alter_fk, fp_alter_pk; +DROP FUNCTION fp_alter_from_trigger(); +DROP ROLE regress_fp_alter_role; diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index a93e81b42bc..4009626bf14 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -3032,3 +3032,47 @@ 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; + +-- ALTER TABLE ... ADD FOREIGN KEY run from inside an AFTER trigger. The +-- validation scan must check each row itself rather than batching, so the +-- orphan row is caught, the ALTER fails, and the constraint is never created. +-- +-- Row-level security on the referenced table forces the row-at-a-time path; +-- RI_Initial_Check() cannot use its single query when RLS applies. +CREATE ROLE regress_fp_alter_role; +CREATE TABLE fp_alter_pk (id int PRIMARY KEY); +INSERT INTO fp_alter_pk VALUES (1); +ALTER TABLE fp_alter_pk ENABLE ROW LEVEL SECURITY; +CREATE POLICY fp_alter_pk_all ON fp_alter_pk USING (true); +GRANT REFERENCES, SELECT ON fp_alter_pk TO regress_fp_alter_role; + +CREATE TABLE fp_alter_fk (a int); +INSERT INTO fp_alter_fk VALUES (1), (999); +ALTER TABLE fp_alter_fk OWNER TO regress_fp_alter_role; + +CREATE TABLE fp_alter_outer (a int); +ALTER TABLE fp_alter_outer OWNER TO regress_fp_alter_role; + +CREATE FUNCTION fp_alter_from_trigger() RETURNS trigger +LANGUAGE plpgsql AS $$ +BEGIN + BEGIN + EXECUTE 'ALTER TABLE fp_alter_fk ADD CONSTRAINT fp_alter_bad_fk ' + 'FOREIGN KEY (a) REFERENCES fp_alter_pk(id)'; + EXCEPTION WHEN others THEN + RAISE; + END; + RETURN NEW; +END$$; +CREATE TRIGGER fp_alter_trg AFTER INSERT ON fp_alter_outer + FOR EACH ROW EXECUTE FUNCTION fp_alter_from_trigger(); + +SET ROLE regress_fp_alter_role; +INSERT INTO fp_alter_outer VALUES (1); +RESET ROLE; +SELECT conname, convalidated FROM pg_constraint WHERE conname = 'fp_alter_bad_fk'; +TABLE fp_alter_fk; + +DROP TABLE fp_alter_outer, fp_alter_fk, fp_alter_pk; +DROP FUNCTION fp_alter_from_trigger(); +DROP ROLE regress_fp_alter_role; -- 2.47.3