Re: Conflict detection for update_deleted in logical replication

On Wed, Jun 25, 2025 at 8:38 AM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)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.