From 5a7d577f43780eee7e07ded26aff863d777c79c6 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Fri, 26 Jun 2026 08:18:18 +0900 Subject: [PATCH v2] Hardwire RI fast-path end-of-xact cleanup into xact.c Commit b7b27eb41a5, which added foreign-key fast-path batching to ri_triggers.c, registered ri_FastPathXactCallback() via RegisterXactCallback() to clear the fast-path batching state at end of transaction. RegisterXactCallback() is documented as intended for dynamically loaded modules; built-in code is supposed to hardwire its end-of-xact hooks into xact.c, mainly so callback ordering can be controlled where it matters (see the header comment on RegisterXactCallback()). Convert the callback into a plain AtEOXact_RI() function and call it directly from CommitTransaction(), PrepareTransaction() and AbortTransaction(), alongside the other AtEOXact_* cleanup steps, and drop the RegisterXactCallback() registration. Like the other AtEOXact_* routines, AtEOXact_RI() takes an isCommit argument and treats the two paths differently. On commit or prepare the fast-path cache must already have been flushed and torn down by the after-trigger batch callback, so a surviving cache indicates a trigger batch was never flushed -- which would have silently skipped FK checks -- and draws an Assert plus a WARNING. On abort a surviving cache is expected (a flush may have errored out partway) and is simply reset. There is no ordering dependency here: AtEOXact_RI() only resets backend-local static state (the cache pointer, the callback-registered flag, and the in-flush guard). It touches no relations, locks, buffers or catalogs, so its position relative to ResourceOwnerRelease() and the surrounding AtEOXact_* calls does not matter. On a normal commit the fast-path cache has already been flushed and torn down by ri_FastPathEndBatch() (an AfterTriggerBatchCallback fired from AfterTriggerFireDeferred(), well before any end-of-xact callback), so the reset is a no-op; its real job is the abort path, where teardown may not have run and the static pointers would otherwise dangle into the next transaction. The cache memory itself lives in TopTransactionContext and is freed by the end-of-transaction memory-context reset on both paths. The companion RegisterSubXactCallback() use from b7b27eb41a5 was already removed by commit 4113873a, which confined fast-path batching to the top transaction level, so only the RegisterXactCallback() use remained. Reported-by: Bertrand Drouvot Reviewed-by: Bertrand Drouvot Discussion: https://postgr.es/m/ajypPeEWceXRGAEW@bdtpg --- src/backend/access/transam/xact.c | 3 ++ src/backend/utils/adt/ri_triggers.c | 58 +++++++++++++++++++++-------- src/include/commands/trigger.h | 2 + 3 files changed, 47 insertions(+), 16 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index de4cf96eaa2..3a89149016f 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -2514,6 +2514,7 @@ CommitTransaction(void) AtEOXact_Files(true); AtEOXact_ComboCid(); AtEOXact_HashTables(true); + AtEOXact_RI(true); AtEOXact_PgStat(true, is_parallel_worker); AtEOXact_Snapshot(true, false); AtEOXact_ApplyLauncher(true); @@ -2809,6 +2810,7 @@ PrepareTransaction(void) AtEOXact_Files(true); AtEOXact_ComboCid(); AtEOXact_HashTables(true); + AtEOXact_RI(true); /* don't call AtEOXact_PgStat here; we fixed pgstat state above */ AtEOXact_Snapshot(true, true); /* we treat PREPARE as ROLLBACK so far as waking workers goes */ @@ -3039,6 +3041,7 @@ AbortTransaction(void) AtEOXact_Files(false); AtEOXact_ComboCid(); AtEOXact_HashTables(false); + AtEOXact_RI(false); AtEOXact_PgStat(false, is_parallel_worker); AtEOXact_ApplyLauncher(false); AtEOXact_LogicalRepWorkers(false); diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 44129a35c08..bf54f9b4592 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -228,7 +228,7 @@ typedef struct RI_CompareHashEntry * relations are held open with locks for the transaction duration, preventing * relcache invalidation. The entry itself is torn down at batch end by * ri_FastPathEndBatch(); on abort, ResourceOwner releases the cached - * relations and the XactCallback NULLs the static cache pointer to prevent + * relations and AtEOXact_RI() NULLs the static cache pointer to prevent * any subsequent access. */ typedef struct RI_FastPathEntry @@ -4187,8 +4187,8 @@ RI_FKey_trigger_type(Oid tgfoid) * Registered as an AfterTriggerBatchCallback. Note: the flush can * do real work (CCI, security context switch, index probes) and can * throw ERROR on a constraint violation. If that happens, - * ri_FastPathTeardown never runs; ResourceOwner + XactCallback - * handle resource cleanup on the abort path. + * ri_FastPathTeardown never runs; ResourceOwner releases the cached + * relations and AtEOXact_RI() resets the static state on the abort path. */ static void ri_FastPathEndBatch(void *arg) @@ -4273,15 +4273,47 @@ ri_FastPathTeardown(void) ri_fastpath_callback_registered = false; } -static bool ri_fastpath_xact_callback_registered = false; - -static void -ri_FastPathXactCallback(XactEvent event, void *arg) +/* + * AtEOXact_RI + * Reset fast-path batching state at end of transaction. + * + * Called from CommitTransaction() and PrepareTransaction() with isCommit + * true, and from AbortTransaction() with isCommit false. + * + * By the time we get here on a clean commit or prepare, the fast-path cache + * has already been flushed and torn down by ri_FastPathEndBatch() (an + * AfterTriggerBatchCallback fired from AfterTriggerFireDeferred(), well before + * this point), so the static pointers are already clear and the reset below is + * a no-op. A surviving cache at commit means a trigger batch was never + * flushed, which would have silently skipped FK checks, so we complain. + * + * On abort, ri_FastPathEndBatch()/ri_FastPathTeardown() may not have run (a + * flush can error out partway): the ResourceOwner releases the cached + * relations and the TopTransactionContext reset frees the cache memory, but + * the process-local static pointers below would dangle into the next + * transaction. This resets them so they don't. + * + * The reset touches only backend-local static state (no relations, locks, + * buffers or catalog access), so it has no ordering dependency on the + * surrounding ResourceOwnerRelease() / AtEOXact_* steps. + */ +void +AtEOXact_RI(bool isCommit) { /* - * On abort, ResourceOwner already released relations; on commit, - * ri_FastPathTeardown already ran. Either way, just NULL the static - * pointers so they don't dangle into the next transaction. + * The cache must be empty on a clean commit or prepare; a survivor means + * a trigger batch went unflushed. Assert for assert-enabled builds and, + * since the transaction is already committed by now and FK checks may + * have been skipped, also warn in production builds. + */ + Assert(ri_fastpath_cache == NULL || !isCommit); + if (isCommit && ri_fastpath_cache != NULL) + elog(WARNING, "RI fast-path cache not flushed at end of transaction"); + + /* + * Clear the static pointers/flags. The cache memory lives in + * TopTransactionContext and is freed by the end-of-transaction + * memory-context reset; here we only drop the references to it. */ ri_fastpath_cache = NULL; ri_fastpath_callback_registered = false; @@ -4316,12 +4348,6 @@ ri_FastPathGetEntry(const RI_ConstraintInfo *riinfo, Relation fk_rel) { HASHCTL ctl; - if (!ri_fastpath_xact_callback_registered) - { - RegisterXactCallback(ri_FastPathXactCallback, NULL); - ri_fastpath_xact_callback_registered = true; - } - ctl.keysize = sizeof(Oid); ctl.entrysize = sizeof(RI_FastPathEntry); ctl.hcxt = TopTransactionContext; diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h index 1d9869973c0..0c3d485abf4 100644 --- a/src/include/commands/trigger.h +++ b/src/include/commands/trigger.h @@ -310,4 +310,6 @@ extern void RegisterAfterTriggerBatchCallback(AfterTriggerBatchCallback callback void *arg); extern bool AfterTriggerIsActive(void); +extern void AtEOXact_RI(bool isCommit); + #endif /* TRIGGER_H */ -- 2.47.3