Horton

Re: Conflict detection for update_deleted in logical replication

Amit Kapila <amit.kapila16@gmail.com>

View thread View on postgr.es
From: Amit Kapila <amit.kapila16@gmail.com>
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Cc: shveta malik <shveta.malik@gmail.com>, vignesh C <vignesh21@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, Nisha Moond <nisha.moond412@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2025-06-25T11:26:41Z
Lists: pgsql-hackers 
On Wed, Jun 25, 2025 at 8:38 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> Here is the V41 patch set which includes the following changes:
>

Few comments on 0004
===================
1.
+
+# Remember the next transaction ID to be assigned
+my $next_xid = $node_A->safe_psql('postgres', "SELECT txid_current() + 1;");
+
+# Confirm that the xmin value is updated
+ok( $node_A->poll_query_until(
+ 'postgres',
+ "SELECT xmin = $next_xid from pg_replication_slots WHERE slot_name =
'pg_conflict_detection'"
+ ),
+ "the xmin value of slot 'pg_conflict_detection' is updated on Node A");
+

Why use an indirect way to verify that the vacuum can now remove rows?
Even if we want to check that the conflict slot is getting updated
properly, we should verify that the vacuum has removed the deleted
rows. Also, please improve comments for this test, as it is not very
clear why you are expecting the latest xid value of conflict_slot.

2.
+# Alter failover for enabled subscription
+my ($cmdret, $stdout, $stderr) = $node_A->psql('postgres',
+ "ALTER SUBSCRIPTION $subname_AB SET (retain_conflict_info = true)");
+ok( $stderr =~
+   /ERROR:  cannot set option \"retain_conflict_info\" for enabled
subscription/,
+ "altering retain_conflict_info is not allowed for enabled subscription");
+
+# Disable the subscription
+($cmdret, $stdout, $stderr) = $node_A->psql('postgres',
+ "ALTER SUBSCRIPTION $subname_AB DISABLE;");
+ok( $stderr =~
+   /WARNING:  deleted rows to detect conflicts would not be removed
until the subscription is enabled/,
+ "A warning is raised on disabling the subscription if
retain_conflict_info is enabled");
+
+# Alter failover for disabled subscription
+($cmdret, $stdout, $stderr) = $node_A->psql('postgres',
+ "ALTER SUBSCRIPTION $subname_AB SET (retain_conflict_info = true);");
+ok( $stderr =~
+   /NOTICE:  deleted rows to detect conflicts would not be removed
until the subscription is enabled/,
+ "altering retain_conflict_info is allowed for disabled subscription");

In all places, the comments use failover as an option name, whereas it
is testing retain_conflict_info.

3. It is better to merge the 0004 into 0001 as it tests the core part
of the functionality added by 0001.

-- 
With Regards,
Amit Kapila.

Commits

  1. Fix intermittent BF failures in 035_conflicts.

  2. Resume conflict-relevant data retention automatically.

  3. Fix intermittent test failure introduced in 6456c6e2c4.

  4. Fix Coverity issue reported in commit a850be2fe.

  5. Add test to prevent premature removal of conflict-relevant data.

  6. Post-commit review fixes for 228c370868.

  7. Add max_retention_duration option to subscriptions.

  8. Detect and report update_deleted conflicts.

  9. Preserve conflict-relevant data during logical replication.

  10. Integrate FullTransactionIds deeper into two-phase code

  11. Improve checks for GUC recovery_target_timeline

  12. Prevent excessive delays before launching new logrep workers.

  13. Keep WAL segments by slot's last saved restart LSN

  14. Rework some code handling pg_subscription data in psql and pg_dump

  15. Use generateClonedIndexStmt to propagate CREATE INDEX to partitions.