On Thu, Jul 9, 2026 at 4:37 AM Amit Langote <[email protected]> wrote: > The resource-owner handling turned out simpler than I expected: because > AtEOSubXact_RI() runs after the subtransaction's ResourceOwnerRelease(), > it only forgets the aborting level's cache entries and never closes their > relations itself, so a batch flush that errors partway through inside a > subxact is cleaned up by the ResourceOwner on the way out.
I kept testing this with my AI harness and found one remaining issue. The ResourceOwner releases the relations, but the slots and flush context live in TopTransactionContext. After 1,000 caught FK violations in one transaction, master retains 1,000 contexts (16 MB) until transaction end. Attached fixes it by releasing the entry before ResourceOwner cleanup, using the owner that acquired its references. It also records the owner and subtransaction before initialization can fail. The regression test covers a PL/pgSQL exception block and an explicit savepoint. Tested on master ceae3099: regression, isolation and injection-point suites pass. The patch applies to current master a625fc5. PG19 no longer has this batching code. Thanks, Nik
From 0cf995d872a4f55820440802bd03288a81fd9a07 Mon Sep 17 00:00:00 2001 From: Nikolay Samokhvalov <[email protected]> Date: Mon, 14 Sep 2026 16:46:20 -0700 Subject: [PATCH] Release RI fast-path entry resources on subtransaction abort AtEOSubXact_RI() removes failed batches from the cache after resource-owner cleanup, but leaves their tuple slots and flush contexts allocated until top-level transaction end. Repeated caught foreign-key violations therefore retain one flush context per failed batch. Run the abort cleanup before ResourceOwnerRelease() and explicitly release each removed entry's resources, using the same helper as normal teardown. Remember the resource owner that acquired those references, since an entry can belong to a portal below the aborting subtransaction's resource owner. Record the owner and subtransaction before acquiring resources so that partially initialized entries are also removed on abort. Extend the existing failed-batch regression test to check that its flush context was removed, and exercise cleanup below an explicit savepoint. The batching code remains in master but was removed from REL_19_STABLE. --- src/backend/access/transam/xact.c | 3 +- src/backend/utils/adt/ri_triggers.c | 63 ++++++++++++++--------- src/test/regress/expected/foreign_key.out | 24 +++++++++ src/test/regress/sql/foreign_key.sql | 19 +++++++ 4 files changed, 85 insertions(+), 24 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index aca92507ebd..30eba17c245 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -5391,6 +5391,8 @@ AbortSubTransaction(void) CallSubXactCallbacks(SUBXACT_EVENT_ABORT_SUB, s->subTransactionId, s->parent->subTransactionId); + AtEOSubXact_RI(false, s->subTransactionId, + s->parent->subTransactionId); ResourceOwnerRelease(s->curTransactionOwner, RESOURCE_RELEASE_BEFORE_LOCKS, @@ -5419,7 +5421,6 @@ 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 c46f789ed14..b9a753f88a0 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -55,6 +55,7 @@ #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/rel.h" +#include "utils/resowner.h" #include "utils/rls.h" #include "utils/ruleutils.h" #include "utils/snapmgr.h" @@ -281,6 +282,7 @@ typedef struct RI_FastPathEntry TupleTableSlot *pk_slot; TupleTableSlot *fk_slot; MemoryContext flush_cxt; /* short-lived context for per-flush work */ + ResourceOwner resowner; /* owner of relation and tuple descriptor refs */ /* * TODO: batch[] is HeapTuple[] because the AFTER trigger machinery @@ -298,7 +300,7 @@ typedef struct RI_FastPathEntry bool flushing; /* - * Subtransaction whose resource owner opened this entry's relations. + * Subtransaction in which this entry's resources were acquired. * AtEOSubXact_RI() drops only entries matching an aborting subxact, so a * subxact abort during outer-level trigger firing leaves the outer batch * intact. @@ -414,6 +416,7 @@ pg_noreturn static void ri_ReportViolation(const RI_ConstraintInfo *riinfo, static RI_FastPathEntry *ri_FastPathGetEntry(RI_ConstraintInfo *riinfo, Relation fk_rel); static void ri_FastPathEndBatch(void *arg); +static void ri_FastPathReleaseEntry(RI_FastPathEntry *entry); static void ri_FastPathTeardown(int depth); @@ -4463,6 +4466,27 @@ ri_FastPathEndBatch(void *arg) ri_FastPathTeardown(my_depth); } +/* Release resources owned by one fast-path cache entry. */ +static void +ri_FastPathReleaseEntry(RI_FastPathEntry *entry) +{ + ResourceOwner save_resowner = CurrentResourceOwner; + + /* Abort cleanup can run outside the portal that owns these references. */ + CurrentResourceOwner = entry->resowner; + if (entry->idx_rel) + index_close(entry->idx_rel, NoLock); + if (entry->pk_rel) + table_close(entry->pk_rel, NoLock); + if (entry->pk_slot) + ExecDropSingleTupleTableSlot(entry->pk_slot); + if (entry->fk_slot) + ExecDropSingleTupleTableSlot(entry->fk_slot); + if (entry->flush_cxt) + MemoryContextDelete(entry->flush_cxt); + CurrentResourceOwner = save_resowner; +} + /* * ri_FastPathTeardown * Release and remove the cached entries of one firing cycle, and drop @@ -4487,16 +4511,7 @@ ri_FastPathTeardown(int depth) { if (entry->key.query_depth != depth) continue; - if (entry->idx_rel) - index_close(entry->idx_rel, NoLock); - if (entry->pk_rel) - table_close(entry->pk_rel, NoLock); - if (entry->pk_slot) - ExecDropSingleTupleTableSlot(entry->pk_slot); - if (entry->fk_slot) - ExecDropSingleTupleTableSlot(entry->fk_slot); - if (entry->flush_cxt) - MemoryContextDelete(entry->flush_cxt); + ri_FastPathReleaseEntry(entry); hash_search(ri_fastpath_cache, &entry->key, HASH_REMOVE, NULL); } @@ -4580,9 +4595,9 @@ 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(). + * Called from CommitSubTransaction() with isCommit true after the + * subtransaction's ResourceOwnerRelease(), and from AbortSubTransaction() + * with isCommit false before ResourceOwnerRelease(). * * Fast-path cache entries are normally flushed and removed at the end of * their trigger-firing cycle, and the cache is destroyed when its last entry @@ -4590,14 +4605,11 @@ AtEOXact_RI(bool isCommit) * * 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 contains entries whose relations - * were opened under this subtransaction's resource owner. That owner has - * just released those relations, making the entries stale. Remove those - * entries so a later firing cycle cannot reuse them. Entries belonging to - * outer subtransactions remain valid and are preserved. - * - * The remaining slot storage and per-entry flush contexts are reclaimed when - * TopTransactionContext is reset at top-level transaction end. + * teardown was skipped, so the cache still contains entries whose resources + * were acquired in this subtransaction, possibly under a portal's resource + * owner. Release and remove those entries before resource-owner cleanup so a + * later firing cycle cannot reuse them. Entries belonging to outer + * subtransactions remain valid and are preserved. */ void AtEOSubXact_RI(bool isCommit, SubTransactionId mySubid, @@ -4630,7 +4642,10 @@ AtEOSubXact_RI(bool isCommit, SubTransactionId mySubid, entry->subid = parentSubid; } else + { + ri_FastPathReleaseEntry(entry); hash_search(ri_fastpath_cache, &entry->key, HASH_REMOVE, NULL); + } } /* If that emptied the cache, drop it so the next batch starts clean. */ @@ -4693,6 +4708,9 @@ ri_FastPathGetEntry(RI_ConstraintInfo *riinfo, Relation fk_rel) */ memset(((char *) entry) + offsetof(RI_FastPathEntry, pk_rel), 0, sizeof(RI_FastPathEntry) - offsetof(RI_FastPathEntry, pk_rel)); + /* Make even a partially initialized entry safe for abort cleanup. */ + entry->subid = GetCurrentSubTransactionId(); + entry->resowner = CurrentResourceOwner; oldcxt = MemoryContextSwitchTo(TopTransactionContext); @@ -4788,7 +4806,6 @@ ri_FastPathGetEntry(RI_ConstraintInfo *riinfo, Relation fk_rel) entry->flushing = false; entry->batch_count = 0; - entry->subid = GetCurrentSubTransactionId(); } else { diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index 8d81240f1c6..78da1da130b 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -3878,6 +3878,12 @@ BEGIN RAISE NOTICE 'caught fk violation'; END; + -- The failed batch's per-entry context must be gone with its cache entry. + IF EXISTS (SELECT FROM pg_backend_memory_contexts + WHERE name = 'RI fast path flush temporary context') THEN + RAISE EXCEPTION 'failed RI batch leaked its memory context'; + END IF; + -- Reuse the same FK with a full batch in the same transaction. The -- entry must be empty after the caught violation: no stale rows from the -- rolled-back batch (in particular no 999), and no array overflow. @@ -3890,6 +3896,24 @@ SELECT count(*), max(a) FROM fp_reentry_fk2; -- 64 rows, max 1 64 | 1 (1 row) +-- Exercise the same cleanup when the entry's resources belong to a portal +-- below an explicit savepoint's resource owner. +BEGIN; +SAVEPOINT fp_savepoint; +INSERT INTO fp_reentry_fk2 + SELECT CASE WHEN g = 64 THEN 999 ELSE 1 END + FROM generate_series(1, 64) g; +ERROR: insert or update on table "fp_reentry_fk2" violates foreign key constraint "fp_reentry_fk2_a_fkey" +DETAIL: Key (a)=(999) is not present in table "fp_reentry_pk2". +ROLLBACK TO fp_savepoint; +SELECT count(*) FROM pg_backend_memory_contexts + WHERE name = 'RI fast path flush temporary context'; + count +------- + 0 +(1 row) + +COMMIT; DROP TABLE fp_reentry_fk2, fp_reentry_pk2; -- Subtransaction abort during after-trigger firing must not drop FK checks -- for rows buffered earlier in the same statement. Batching is confined to diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index 184d9efdc97..dc756f873bf 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -2836,12 +2836,31 @@ BEGIN RAISE NOTICE 'caught fk violation'; END; + -- The failed batch's per-entry context must be gone with its cache entry. + IF EXISTS (SELECT FROM pg_backend_memory_contexts + WHERE name = 'RI fast path flush temporary context') THEN + RAISE EXCEPTION 'failed RI batch leaked its memory context'; + END IF; + -- Reuse the same FK with a full batch in the same transaction. The -- entry must be empty after the caught violation: no stale rows from the -- rolled-back batch (in particular no 999), and no array overflow. INSERT INTO fp_reentry_fk2 SELECT 1 FROM generate_series(1, 64); END$$; SELECT count(*), max(a) FROM fp_reentry_fk2; -- 64 rows, max 1 + +-- Exercise the same cleanup when the entry's resources belong to a portal +-- below an explicit savepoint's resource owner. +BEGIN; +SAVEPOINT fp_savepoint; +INSERT INTO fp_reentry_fk2 + SELECT CASE WHEN g = 64 THEN 999 ELSE 1 END + FROM generate_series(1, 64) g; +ROLLBACK TO fp_savepoint; +SELECT count(*) FROM pg_backend_memory_contexts + WHERE name = 'RI fast path flush temporary context'; +COMMIT; + DROP TABLE fp_reentry_fk2, fp_reentry_pk2; -- Subtransaction abort during after-trigger firing must not drop FK checks base-commit: ceae3099da7b43d1130267f6904da3c22f1b49eb -- 2.50.1 (Apple Git-155)
