On Friday, September 5, 2025 2:01 PM shveta malik <[email protected]> wrote: > > On Thu, Sep 4, 2025 at 3:30 PM Zhijie Hou (Fujitsu) > <[email protected]> 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?
I think it's OK to directly refer to commitproc, so changed.
>
> ~~
>
> Few trivial comments for 002:
Thanks for the comments.
> 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.
Added.
>
> 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..
Changed.
>
> 3)
> +# Create a publisher node. Disable autovacuum to stablized the tests related
> to
> +# manual VACUUM and transaction ID.
>
> to stablized --> to stabilize
Fixed.
>
> 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
Fixed.
Here are v2 patches which addressed above comments.
Best Regards,
Hou zj
v2-0002-Add-a-race-condition-test.patch
Description: v2-0002-Add-a-race-condition-test.patch
v2-0001-Fix-conflict-relevant-data-retention-for-prepared.patch
Description: v2-0001-Fix-conflict-relevant-data-retention-for-prepared.patch
