From b13d506f12308a8b8696dc1fd255ad3fd1d79283 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Wed, 19 Aug 2026 21:59:07 +0900 Subject: [PATCH v3 3/3] Track RI fast-path FK-check batches per subtransaction Commit 4113873 confined RI fast-path batching to the top transaction level to avoid mishandling the batch cache on subtransaction abort. That disabled batching for a foreign-key load wrapped in a savepoint (e.g. "BEGIN; SAVEPOINT s; COPY fk_table FROM ..."), a surprising performance cliff, and departed from the usual per-subtransaction resource handling. Handle the cache per subtransaction instead. Add AtEOSubXact_RI(), called from CommitSubTransaction() and AbortSubTransaction() after ResourceOwnerRelease(). On abort, it removes only entries opened by the ending subtransaction, whose resources have just been released, while leaving entries opened by an outer level intact. Thus, an inner subtransaction abort during outer-level trigger firing does not discard the outer statement's batch. On commit, no matching entry is expected, because its batch should already have been flushed at statement end. Each entry records the subtransaction that opened its resources. A fast-path batch is filled and flushed within a single trigger-firing cycle, so every row added to an entry must come from the subtransaction that created it. AtEOSubXact_RI() relies on this invariant to identify an aborting subtransaction's entries by the subid stamped at entry creation. Assert the invariant in ri_FastPathBatchAdd(). Add regression coverage for nested firing with different constraints inside a subtransaction and for nested firing of the same constraint. Reported-by: Noah Misch Reported-by: Nikolay Samokhvalov Discussion: https://postgr.es/m/20260705222115.be.noahmisch@microsoft.com Backpatch-through: 19 --- src/backend/access/transam/xact.c | 2 + src/backend/utils/adt/ri_triggers.c | 111 ++++++++++++++++++++-- src/include/commands/trigger.h | 2 + src/test/regress/expected/foreign_key.out | 70 ++++++++++++++ src/test/regress/sql/foreign_key.sql | 57 +++++++++++ 5 files changed, 234 insertions(+), 8 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 5e08415e50c..aca92507ebd 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -5244,6 +5244,7 @@ CommitSubTransaction(void) s->parent->subTransactionId); AtEOSubXact_HashTables(true, s->nestingLevel); AtEOSubXact_PgStat(true, s->nestingLevel); + AtEOSubXact_RI(true, s->subTransactionId, s->parent->subTransactionId); AtSubCommit_Snapshot(s->nestingLevel); /* @@ -5418,6 +5419,7 @@ AbortSubTransaction(void) s->parent->subTransactionId); AtEOSubXact_HashTables(false, s->nestingLevel); AtEOSubXact_PgStat(false, s->nestingLevel); + AtEOSubXact_RI(false, s->subTransactionId, s->parent->subTransactionId); AtSubAbort_Snapshot(s->nestingLevel); } diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 6863ebb8bce..e320435509c 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -287,6 +287,14 @@ typedef struct RI_FastPathEntry * re-entrant ri_FastPathBatchAdd from user code run during the flush. */ bool flushing; + + /* + * Subtransaction whose resource owner opened this entry's relations. + * AtEOSubXact_RI() drops only entries matching an aborting subxact, so a + * subxact abort during outer-level trigger firing leaves the outer batch + * intact. + */ + SubTransactionId subid; } RI_FastPathEntry; /* @@ -512,9 +520,7 @@ RI_FKey_check(TriggerData *trigdata) */ if (ri_fastpath_is_applicable(riinfo)) { - if (AfterTriggerIsActive() && - GetCurrentTransactionNestLevel() == 1 && - !ri_fastpath_flushing) + if (AfterTriggerIsActive() && !ri_fastpath_flushing) { /* Batched path: buffer and probe in groups */ ri_FastPathBatchAdd(riinfo, fk_rel, newslot); @@ -522,15 +528,11 @@ RI_FKey_check(TriggerData *trigdata) else { /* - * Per-row path, used when batching is not safe or not applicable: + * Per-row path, used when batching is not applicable: * * - ALTER TABLE validation, where no after-trigger firing is * active; * - * - any FK check inside a subtransaction, since the batch cache - * is confined to the top transaction level (it cannot be cleanly - * unwound on subxact abort); - * * - a re-entrant check from user cast/operator code running * during a batch flush, since adding a cache entry while * ri_FastPathEndBatch is iterating the cache could leave it @@ -2969,6 +2971,14 @@ ri_FastPathBatchAdd(RI_ConstraintInfo *riinfo, return; } + /* + * A batch is filled and flushed within a single trigger-firing cycle, so + * every row added to an entry comes from the subtransaction that created + * it. AtEOSubXact_RI() relies on this to identify an aborting + * subtransaction's entries by the subid stamped at entry creation. + */ + Assert(fpentry->subid == GetCurrentSubTransactionId()); + /* * Buffer the row. A full batch is flushed below and re-entry is handled * above, so there is always room here; the bounds check just guards the @@ -4480,6 +4490,90 @@ AtEOXact_RI(bool isCommit) } } +/* + * AtEOSubXact_RI + * Reset fast-path batching state at subtransaction end. + * + * Called from CommitSubTransaction() with isCommit true and from + * AbortSubTransaction() with isCommit false, in both cases after the + * subtransaction's ResourceOwnerRelease(). + * + * The fast-path cache is created and torn down within a single trigger-firing + * batch (ri_FastPathEndBatch(), an AfterTriggerBatchCallback), so at a normal + * subtransaction boundary it is already empty and this is a no-op. + * + * The exception is a batch flush that errors out partway and is caught by this + * subtransaction (e.g. a PL/pgSQL EXCEPTION block): ri_FastPathEndBatch()'s + * teardown was skipped, so the cache still points at entries whose relations + * were opened under this subtransaction's resource owner. That owner has just + * released those relations (this runs after ResourceOwnerRelease()), so the + * entries are now stale. Drop the cache so a later statement in the parent + * doesn't reuse it. Like AtEOXact_RI(), this touches only backend-local state + * and the hash table's own memory -- no relations, locks or buffers, which the + * ResourceOwner already handled. The entries' slots and flush contexts live in + * TopTransactionContext and are freed by its end-of-transaction reset. + */ +void +AtEOSubXact_RI(bool isCommit, SubTransactionId mySubid, + SubTransactionId parentSubid) +{ + HASH_SEQ_STATUS status; + RI_FastPathEntry *entry; + long remaining; + + if (ri_fastpath_cache == NULL) + return; + + /* + * Drop only entries whose relations were opened under the ending + * subtransaction's resource owner. On abort that owner has just released + * those relations (this runs after ResourceOwnerRelease()), so the entry + * is stale and must go, but entries opened by an outer level -- e.g. an + * outer statement's batch, mid-build when an inner subxact fired and + * aborted -- must be left untouched. + * + * On commit the entry, if any, would have been flushed and torn down at + * the end of its statement (ri_FastPathEndBatch()); reaching here with a + * matching entry is not expected, but reassign it to the parent so it is + * still cleaned up, rather than leaving it under a vanished subxact id. + * + * We touch no relations, locks or buffers -- the ResourceOwner handled + * those. The entry's slots and flush context are memory in + * TopTransactionContext, freed at end-of-transaction reset; we only + * remove the hash entry so it is not reused or torn down again. + */ + hash_seq_init(&status, ri_fastpath_cache); + while ((entry = hash_seq_search(&status)) != NULL) + { + if (entry->subid != mySubid) + continue; + + if (isCommit) + { + /* + * A committing subxact's entry should already have been flushed + * and torn down at its statement's end (ri_FastPathEndBatch()), + * so we don't expect to find one here. If we do, reassign it to + * the parent so it's still cleaned up rather than left under a + * subxact id that no longer exists. + */ + Assert(false); + entry->subid = parentSubid; + } + else + hash_search(ri_fastpath_cache, &entry->key, HASH_REMOVE, NULL); + } + + /* If that emptied the cache, drop it so the next batch starts clean. */ + remaining = hash_get_num_entries(ri_fastpath_cache); + if (remaining == 0) + { + hash_destroy(ri_fastpath_cache); + ri_fastpath_cache = NULL; + ri_fastpath_flushing = false; + } +} + /* * ri_FastPathGetEntry * Look up or create a per-batch cache entry for the given constraint. @@ -4613,6 +4707,7 @@ ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo, Relation fk_rel) entry->flushing = false; entry->batch_count = 0; + entry->subid = GetCurrentSubTransactionId(); } return entry; diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h index 1f268f87957..fecdb785f35 100644 --- a/src/include/commands/trigger.h +++ b/src/include/commands/trigger.h @@ -312,5 +312,7 @@ extern bool AfterTriggerIsActive(void); extern int AfterTriggerCurrentQueryDepth(void); extern void AtEOXact_RI(bool isCommit); +extern void AtEOSubXact_RI(bool isCommit, SubTransactionId mySubid, + SubTransactionId parentSubid); #endif /* TRIGGER_H */ diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index d7116084d8f..bb69ba051c0 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -3951,6 +3951,76 @@ ROLLBACK; RESET ROLE; DROP TABLE fpav_fk, fpav_pk, fpav_cv_fk, fpav_cv_pk; DROP ROLE regress_fpav_role; +-- Re-entrant fast-path check inside a committing subtransaction. An AFTER +-- trigger on one FK table runs FK DML on a second FK table inside a PL/pgSQL +-- BEGIN ... EXCEPTION block, so the inner check batches in its own +-- trigger-firing cycle nested in the outer check's. The inner cycle must +-- register its own end-of-batch callback and flush -- otherwise its FK check +-- is skipped (an orphan commits) and its relations leak. +CREATE TABLE fp_inner_pk (id int PRIMARY KEY); +INSERT INTO fp_inner_pk VALUES (1); +CREATE TABLE fp_inner_fk (a int REFERENCES fp_inner_pk (id)); +CREATE TABLE fp_outer_pk (id int PRIMARY KEY); +INSERT INTO fp_outer_pk SELECT g FROM generate_series(1, 64) g; +CREATE FUNCTION fp_reentry_subxact() RETURNS trigger LANGUAGE plpgsql AS $$ +BEGIN + IF NEW.a = 32 THEN + BEGIN + INSERT INTO fp_inner_fk VALUES (999); -- violates; must be caught + EXCEPTION WHEN foreign_key_violation THEN + NULL; + END; + END IF; + RETURN NEW; +END$$; +CREATE TABLE fp_outer_fk (a int REFERENCES fp_outer_pk (id)); +CREATE TRIGGER fp_reentry_subxact_trg AFTER INSERT ON fp_outer_fk + FOR EACH ROW EXECUTE FUNCTION fp_reentry_subxact(); +INSERT INTO fp_outer_fk SELECT g FROM generate_series(1, 64) g; +SELECT count(*) AS outer_rows FROM fp_outer_fk; -- 64, outer batch intact + outer_rows +------------ + 64 +(1 row) + +SELECT count(*) AS inner_rows FROM fp_inner_fk; -- 0, inner check caught + inner_rows +------------ + 0 +(1 row) + +DROP TRIGGER fp_reentry_subxact_trg ON fp_outer_fk; +DROP FUNCTION fp_reentry_subxact(); +DROP TABLE fp_outer_fk, fp_outer_pk, fp_inner_fk, fp_inner_pk; +-- A nested trigger-firing cycle that checks the same constraint must use a +-- separate cache entry. The inner violation is caught by its subtransaction, +-- while the valid outer row remains buffered and is checked normally. +CREATE TABLE fp_same_pk (id int PRIMARY KEY); +INSERT INTO fp_same_pk VALUES (1); +CREATE TABLE fp_same_fk (a int REFERENCES fp_same_pk (id)); +CREATE FUNCTION fp_reentry_same_constraint() RETURNS trigger LANGUAGE plpgsql AS $$ +BEGIN + IF NEW.a = 1 THEN + BEGIN + INSERT INTO fp_same_fk VALUES (999); + EXCEPTION WHEN foreign_key_violation THEN + NULL; + END; + END IF; + RETURN NEW; +END$$; +CREATE TRIGGER fp_reentry_same_constraint_trg AFTER INSERT ON fp_same_fk + FOR EACH ROW EXECUTE FUNCTION fp_reentry_same_constraint(); +INSERT INTO fp_same_fk VALUES (1); +SELECT * FROM fp_same_fk; + a +--- + 1 +(1 row) + +DROP TRIGGER fp_reentry_same_constraint_trg ON fp_same_fk; +DROP FUNCTION fp_reentry_same_constraint(); +DROP TABLE fp_same_fk, fp_same_pk; -- An AFTER trigger runs a query of its own, and that query inserts into a -- second table with a fast-path foreign key. The entry the nested INSERT -- creates belongs to the cursor's portal, which is gone by the time the diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index acd4fa8dea5..22e814eca6f 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -2898,6 +2898,63 @@ RESET ROLE; DROP TABLE fpav_fk, fpav_pk, fpav_cv_fk, fpav_cv_pk; DROP ROLE regress_fpav_role; +-- Re-entrant fast-path check inside a committing subtransaction. An AFTER +-- trigger on one FK table runs FK DML on a second FK table inside a PL/pgSQL +-- BEGIN ... EXCEPTION block, so the inner check batches in its own +-- trigger-firing cycle nested in the outer check's. The inner cycle must +-- register its own end-of-batch callback and flush -- otherwise its FK check +-- is skipped (an orphan commits) and its relations leak. +CREATE TABLE fp_inner_pk (id int PRIMARY KEY); +INSERT INTO fp_inner_pk VALUES (1); +CREATE TABLE fp_inner_fk (a int REFERENCES fp_inner_pk (id)); +CREATE TABLE fp_outer_pk (id int PRIMARY KEY); +INSERT INTO fp_outer_pk SELECT g FROM generate_series(1, 64) g; +CREATE FUNCTION fp_reentry_subxact() RETURNS trigger LANGUAGE plpgsql AS $$ +BEGIN + IF NEW.a = 32 THEN + BEGIN + INSERT INTO fp_inner_fk VALUES (999); -- violates; must be caught + EXCEPTION WHEN foreign_key_violation THEN + NULL; + END; + END IF; + RETURN NEW; +END$$; +CREATE TABLE fp_outer_fk (a int REFERENCES fp_outer_pk (id)); +CREATE TRIGGER fp_reentry_subxact_trg AFTER INSERT ON fp_outer_fk + FOR EACH ROW EXECUTE FUNCTION fp_reentry_subxact(); +INSERT INTO fp_outer_fk SELECT g FROM generate_series(1, 64) g; +SELECT count(*) AS outer_rows FROM fp_outer_fk; -- 64, outer batch intact +SELECT count(*) AS inner_rows FROM fp_inner_fk; -- 0, inner check caught +DROP TRIGGER fp_reentry_subxact_trg ON fp_outer_fk; +DROP FUNCTION fp_reentry_subxact(); +DROP TABLE fp_outer_fk, fp_outer_pk, fp_inner_fk, fp_inner_pk; + +-- A nested trigger-firing cycle that checks the same constraint must use a +-- separate cache entry. The inner violation is caught by its subtransaction, +-- while the valid outer row remains buffered and is checked normally. +CREATE TABLE fp_same_pk (id int PRIMARY KEY); +INSERT INTO fp_same_pk VALUES (1); +CREATE TABLE fp_same_fk (a int REFERENCES fp_same_pk (id)); +CREATE FUNCTION fp_reentry_same_constraint() RETURNS trigger LANGUAGE plpgsql AS $$ +BEGIN + IF NEW.a = 1 THEN + BEGIN + INSERT INTO fp_same_fk VALUES (999); + EXCEPTION WHEN foreign_key_violation THEN + NULL; + END; + END IF; + RETURN NEW; +END$$; +CREATE TRIGGER fp_reentry_same_constraint_trg AFTER INSERT ON fp_same_fk + FOR EACH ROW EXECUTE FUNCTION fp_reentry_same_constraint(); +INSERT INTO fp_same_fk VALUES (1); +SELECT * FROM fp_same_fk; +DROP TRIGGER fp_reentry_same_constraint_trg ON fp_same_fk; +DROP FUNCTION fp_reentry_same_constraint(); +DROP TABLE fp_same_fk, fp_same_pk; + -- An AFTER trigger runs a query of its own, and that query inserts into a -- second table with a fast-path foreign key. The entry the nested INSERT -- creates belongs to the cursor's portal, which is gone by the time the -- 2.47.3