pgsql: Preserve conflict-relevant data during logical replication.

On Wed, Jul 23, 2025 at 03:35:06AM +0000, Amit Kapila wrote:

Preserve conflict-relevant data during logical replication.

Logical replication requires reliable conflict detection to maintain data
consistency across nodes. To achieve this, we must prevent premature
removal of tuples deleted by other origins and their associated commit_ts
data by VACUUM, which could otherwise lead to incorrect conflict reporting
and resolution.

Some of the tests added by this commit are causing blurps in the CI:
https://cfbot.cputube.org/highlights/all.html

Example of one job (triggered it once myself with a separate patch):
https://cirrus-ci.com/task/5397273342902272
[21:15:03.131](0.028s) not ok 7 - altering retain_dead_tuples is
allowed for disabled subscription
[21:18:48.295](225.164s) # poll_query_until timed out executing this
query:
[21:18:48.296](0.000s) not ok 8 - the xmin value of slot
'pg_conflict_detection' is valid on Node A
[21:18:48.306](0.010s) not ok 9 - warn of the possibility of receiving
changes from origins other than the publisher
[21:18:48.412](0.037s) not ok 11 - the deleted column is non-removable
[21:22:29.286](220.874s) # poll_query_until timed out executing this
query:
[21:22:29.287](0.000s) not ok 12 - the xmin value of slot
'pg_conflict_detection' is updated on Node A
[21:22:29.297](0.010s) not ok 13 - the deleted column is removed

The failures happen on FreeBSD as far as I know, that enforces some
rules not used elsewhere:
-c debug_copy_parse_plan_trees=on
-c debug_write_read_parse_plan_trees=on
-c debug_raw_expression_coverage_test=on
-c debug_parallel_query=regress
[...]
CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS

I suspect that reusing these options would help in reproducing the
problem. These are not commonly used in buildfarm animals, reducing
the friction to make the instability show up.
--
Michael

On Thursday, July 24, 2025 9:25 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Jul 23, 2025 at 03:35:06AM +0000, Amit Kapila wrote:

Preserve conflict-relevant data during logical replication.

Logical replication requires reliable conflict detection to maintain
data consistency across nodes. To achieve this, we must prevent
premature removal of tuples deleted by other origins and their
associated commit_ts data by VACUUM, which could otherwise lead to
incorrect conflict reporting and resolution.

Some of the tests added by this commit are causing blurps in the CI:
https://cfbot.cputube.org/highlights/all.html

Example of one job (triggered it once myself with a separate patch):
https://cirrus-ci.com/task/5397273342902272
[21:15:03.131](0.028s) not ok 7 - altering retain_dead_tuples is allowed for
disabled subscription
[21:18:48.295](225.164s) # poll_query_until timed out executing this
query:
[21:18:48.296](0.000s) not ok 8 - the xmin value of slot 'pg_conflict_detection'
is valid on Node A
[21:18:48.306](0.010s) not ok 9 - warn of the possibility of receiving changes
from origins other than the publisher
[21:18:48.412](0.037s) not ok 11 - the deleted column is non-removable
[21:22:29.286](220.874s) # poll_query_until timed out executing this
query:
[21:22:29.287](0.000s) not ok 12 - the xmin value of slot 'pg_conflict_detection'
is updated on Node A
[21:22:29.297](0.010s) not ok 13 - the deleted column is removed

The failures happen on FreeBSD as far as I know, that enforces some rules not
used elsewhere:
-c debug_copy_parse_plan_trees=on
-c debug_write_read_parse_plan_trees=on
-c debug_raw_expression_coverage_test=on
-c debug_parallel_query=regress
[...]
CPPFLAGS: -DRELCACHE_FORCE_RELEASE
-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS

I suspect that reusing these options would help in reproducing the problem.
These are not commonly used in buildfarm animals, reducing the friction to
make the instability show up.

Thanks for reporting the issue!

I confirmed that the test to enable the retain_dead_tuples option for a
disabled subscription failed due to the apply worker for that subscription still
running, which caused the all subsequent tests to fail. To resolve this issue,
we need to ensure the apply worker has stopped when disabling the subscription.

2025-07-23 21:15:03.128 UTC client backend[39133] 035_conflicts.pl LOG: statement: ALTER SUBSCRIPTION tap_sub_a_b SET (retain_dead_tuples = true);
2025-07-23 21:15:03.128 UTC client backend[39133] 035_conflicts.pl ERROR: cannot alter retain_dead_tuples when logical replication worker is still running

Attached is a patch to address this problem. Apart from the reported failure,
there's another place where we did not wait for the worker to stop after
disabling the subscription. Although this hasn't resulted in a test failure so
far, I added wait logic for it in the patch as well for safety.

Best Regards,
Hou zj

On Thu, Jul 24, 2025 at 8:27 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Thursday, July 24, 2025 9:25 AM Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Jul 23, 2025 at 03:35:06AM +0000, Amit Kapila wrote:

Preserve conflict-relevant data during logical replication.

Logical replication requires reliable conflict detection to maintain
data consistency across nodes. To achieve this, we must prevent
premature removal of tuples deleted by other origins and their
associated commit_ts data by VACUUM, which could otherwise lead to
incorrect conflict reporting and resolution.

Some of the tests added by this commit are causing blurps in the CI:
https://cfbot.cputube.org/highlights/all.html

...

I confirmed that the test to enable the retain_dead_tuples option for a
disabled subscription failed due to the apply worker for that subscription still
running, which caused the all subsequent tests to fail. To resolve this issue,
we need to ensure the apply worker has stopped when disabling the subscription.

2025-07-23 21:15:03.128 UTC client backend[39133] 035_conflicts.pl LOG: statement: ALTER SUBSCRIPTION tap_sub_a_b SET (retain_dead_tuples = true);
2025-07-23 21:15:03.128 UTC client backend[39133] 035_conflicts.pl ERROR: cannot alter retain_dead_tuples when logical replication worker is still running

Attached is a patch to address this problem. Apart from the reported failure,
there's another place where we did not wait for the worker to stop after
disabling the subscription. Although this hasn't resulted in a test failure so
far, I added wait logic for it in the patch as well for safety.

The fix looks good to me. I'll push your patch in sometime.

--
With Regards,
Amit Kapila.

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

On Tuesday, September 2, 2025 11:03 PM Robert Haas <robertmhaas@gmail.com> wrote:

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.

Thanks for pointing it out!

I agree that adding a test is valuable to mitigate the risk of future code
changes. We added a similar safeguard for the RecordTransactionCommit() function
by adding Assert(xactStopTimestamp == 0) after marking the DELAY_CHKPT_xxx flag,
and did not do any precautionary check for RecordTransactionCommitPrepared as
the code to acquire the timestamp and setting flag was close by and had explicit
comments.

I'll prepare a test case that uses the injection point and share it in the
original thread.

Best Regards,
Hou zj

On Wed, Sep 3, 2025 at 5:10 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:

On Tuesday, September 2, 2025 11:03 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Jul 23, 2025 at 11:44 PM Amit Kapila <amit.kapila16@gmail.com>
wrote:

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.

Thanks for pointing it out!

I agree that adding a test is valuable to mitigate the risk of future code
changes. We added a similar safeguard for the RecordTransactionCommit() function
by adding Assert(xactStopTimestamp == 0) after marking the DELAY_CHKPT_xxx flag,
and did not do any precautionary check for RecordTransactionCommitPrepared as
the code to acquire the timestamp and setting flag was close by and had explicit
comments.

I'll prepare a test case that uses the injection point and share it in the
original thread.

The test for this case is added in commit
6456c6e2c4ad1cf9752e09cce37bfcfe2190c5e0.

--
With Regards,
Amit Kapila.

On Wed, Sep 10, 2025 at 5:11 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

The test for this case is added in commit
6456c6e2c4ad1cf9752e09cce37bfcfe2190c5e0.

Thanks!

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