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 <bertranddrouvot.pg@gmail.com>
Reviewed-by: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Discussion: https://postgr.es/m/ajypPeEWceXRGAEW@bdtpg

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index de4cf96eaa2caba76474567367ec4a336c5e51c9..3a89149016fe6f8eaf33075c9d561d71b607ad5d 100644 (file)
--- 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 44129a35c0895818d6434f3d05460e350edf86fd..bf54f9b459243fcd207bbd00525739ce83e9f426 100644 (file)
--- 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 1d9869973c094c1e116eff021e29347e8f5be640..0c3d485abf47f6740585c8b920ad8ebab620cf34 100644 (file)
--- 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 */