Re: Conflict detection for update_deleted in logical replication

On Thu, Sep 4, 2025 at 3:30 PM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> Hi,
>
> As reported by Robert[1], it is worth adding a test for the race condition in
> the RecordTransactionCommitPrepared() function to reduce the risk of future code
> changes:
>
> /*
> * 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();
>
> While writing the test, I noticed a bug that conflict-relevant data could be
> prematurely removed before applying prepared transactions on the publisher that
> are in the commit phase. This occurred because GetOldestActiveTransactionId()
> was called on the publisher, which failed to account for the backend executing
> COMMIT PREPARED, as this backend does not have an xid stored in PGPROC.
>
> Since this issue overlaps with the race condition related to timestamp
> acquisition, I've prepared a bug fix along with a test for the race condition.
> The 0001 patch fixes this issue by introducing a new function that iterates over
> global transactions and identifies prepared transactions during the commit
> phase. 0002 added injection points and tap-test to test the bug and timestamp
> acquisition.
>

Thank You for the patches.Verified 001, works well. Just one minor comment:

1)
+ PGPROC *proc = GetPGProcByNumber(gxact->pgprocno);
+ PGPROC *commitproc;
We get first proc and then commitproc. proc is used only to get
databaseId, why don't we get databaseId from commitproc itself?

~~

Few trivial comments for 002:
1)
+# Test that publisher's transactions marked with DELAY_CHKPT_IN_COMMIT prevent
+# concurrently deleted tuples on the subscriber from being removed.

Here shall we also mention something like:
This test also acts as a safeguard to prevent developers from moving
the timestamp acquisition before marking DELAY_CHKPT_IN_COMMIT in
RecordTransactionCommitPrepared.

2)
# This test depends on an injection point to block the transaction commit after
# marking DELAY_CHKPT_IN_COMMIT flag.

Shall we say:
..to block the prepared transaction commit..

3)
+# Create a publisher node. Disable autovacuum to stablized the tests related to
+# manual VACUUM and transaction ID.

to stablized --> to stabilize

4)
+# Confirm that the dead tuple can be removed now
+my ($cmdret, $stdout, $stderr) =
+ $node_subscriber->psql('postgres', qq(VACUUM (verbose) public.tab;));
+
+ok($stderr =~ qr/1 are dead but not yet removable/,
+ 'the deleted column is non-removable');

can be removed now --> cannot be removed now

thanks
Shveta