Re: pgsql: Preserve conflict-relevant data during logical replication.

On Wed, Jul 23, 2025 at 11:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> The fix looks good to me. I'll push your patch in sometime.

The tests in this patch are insufficient to prove that this logic
works properly. I tried with this patch:

diff --git a/src/backend/access/transam/twophase.c
b/src/backend/access/transam/twophase.c
index 7918176fc58..7c1d0ef07b5 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -2325,6 +2325,16 @@ RecordTransactionCommitPrepared(TransactionId xid,
     TimestampTz committs;
     bool        replorigin;

+    /*
+     * Note it is important to set committs value after marking ourselves as
+     * in the commit critical section (DELAY_CHKPT_IN_COMMIT). This is because
+     * we want to ensure all transactions that have acquired commit timestamp
+     * are finished before we allow the logical replication client to advance
+     * its xid which is used to hold back dead rows for conflict detection.
+     * See comments atop worker.c.
+     */
+    committs = GetCurrentTimestamp();
+
     /*
      * Are we using the replication origins feature?  Or, in other words, are
      * we replaying remote actions?
@@ -2344,16 +2354,6 @@ RecordTransactionCommitPrepared(TransactionId xid,
      */
     pg_write_barrier();

-    /*
-     * Note it is important to set committs value after marking ourselves as
-     * in the commit critical section (DELAY_CHKPT_IN_COMMIT). This is because
-     * we want to ensure all transactions that have acquired commit timestamp
-     * are finished before we allow the logical replication client to advance
-     * its xid which is used to hold back dead rows for conflict detection.
-     * See comments atop worker.c.
-     */
-    committs = GetCurrentTimestamp();
-
     /*
      * Emit the XLOG commit record. Note that we mark 2PC commits as
      * potentially having AccessExclusiveLocks since we don't know whether or

If it is in fact important to acquire the commit timestamp after
setting delayChkptFlags, you'd hope this would lead to a test failure,
but it doesn't for me. I understand it probably requires an injection
point to be certain of hitting the race condition, but I think that
would be worth doing. Otherwise, if something gets broken here by
accident, it might be a long time before anyone notices.

--
Robert Haas
EDB: http://www.enterprisedb.com