From d256d0f9a98ed57bffe8b8ad42442de5c5c45d6a Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Fri, 4 Sep 2026 20:34:18 +0900 Subject: [PATCH v4 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 --- src/backend/commands/tablecmds.c | 16 ++----- src/backend/commands/trigger.c | 39 ++++++----------- src/backend/utils/adt/ri_triggers.c | 29 ++++++++++--- 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, 135 insertions(+), 47 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index fd144d783d9..ce866274001 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -14218,8 +14218,8 @@ validateForeignKeyConstraint(char *conname, return; /* - * Scan through each tuple, calling RI_FKey_check_ins (insert trigger) as - * if that tuple had just been inserted. If any of those fail, it should + * Scan through each tuple, calling RI_FKey_check_validate to check that + * it satisfies the constraint. If any of those checks fail, it should * ereport(ERROR) and that's that. */ snapshot = RegisterSnapshot(GetLatestSnapshot()); @@ -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 603d798f320..22b49240a28 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -3940,8 +3940,8 @@ typedef struct AfterTriggersData /* * Incremented around the trigger-firing loops in AfterTriggerEndQuery, - * AfterTriggerFireDeferred, and AfterTriggerSetState. Used by - * AfterTriggerIsActive() to signal that after-trigger firing is active. + * AfterTriggerFireDeferred, and AfterTriggerSetState. Identifies the + * firing cycle a batch callback is registered from. */ int firing_depth; } AfterTriggersData; @@ -6898,13 +6898,14 @@ check_modified_virtual_generated(TupleDesc tupdesc, HeapTuple tuple) /* * RegisterAfterTriggerBatchCallback * Register a function to be called when the current trigger-firing - * batch completes. + * cycle ends. * - * Must be called from within a trigger function's execution context - * (i.e., while afterTriggers state is active). + * Must be called from within a firing cycle, which is what guarantees the + * callback will be invoked: the cycle fires its callbacks before it returns. * - * The callback list is cleared after invocation, so the caller must - * re-register for each new batch if needed. + * A cycle's callbacks are invoked once, at its end, and are not carried into + * any later cycle, so a caller that wants a callback in a later cycle must + * register again there. */ void RegisterAfterTriggerBatchCallback(AfterTriggerBatchCallback callback, @@ -6913,15 +6914,13 @@ RegisterAfterTriggerBatchCallback(AfterTriggerBatchCallback callback, AfterTriggerCallbackItem *item; MemoryContext oldcxt; + Assert(afterTriggers.firing_depth > 0); + Assert(!afterTriggers.firing_batch_callbacks); + /* * Allocate in TopTransactionContext so the item survives for the duration - * of the batch, which may span multiple trigger invocations. - * - * Must be called while afterTriggers is active; callbacks registered - * outside a trigger-firing context would never fire. + * of the cycle, which may span multiple trigger invocations. */ - Assert(afterTriggers.firing_depth > 0); - Assert(!afterTriggers.firing_batch_callbacks); oldcxt = MemoryContextSwitchTo(TopTransactionContext); item = palloc_object(AfterTriggerCallbackItem); item->callback = callback; @@ -6963,20 +6962,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..11396d94c2d 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); @@ -530,8 +530,8 @@ RI_FKey_check(TriggerData *trigdata) /* * Per-row path, used when batching is not applicable: * - * - ALTER TABLE validation, where no after-trigger firing is - * active; + * - ALTER TABLE validation, whose caller passes allow_batch = + * false because no firing cycle will flush its checks; * * - a re-entrant check from user cast/operator code running * during a batch flush, since adding a cache entry while @@ -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 333994e6505..f1ec4dbd75a 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 21ef1931f31..7ffba484739 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