Horton

RE: Conflict detection for update_deleted in logical replication

Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>

View thread View on postgr.es
From: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
To: Amit Kapila <amit.kapila16@gmail.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-26T03:02:01Z
Lists: pgsql-hackers

Attachments

On Wed, Jun 25, 2025 at 7:27 PM Amit Kapila wrote:
> 
> 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.

I agree that testing VACUUM is straightforward. But I think there is a gap
between applying remote changes and updating slot.xmin in the launcher.
Therefore, it's necessary to wait for the launcher to update the slot before
testing whether VACUUM can remove the dead tuple.

I have improved the comments and added the VACUUM test as
suggested after the slot.xmin test.

> 
> 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.

Changed.

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

Merged.

Here is the V41 patch set which includes the following changes:

0001:
* ran pgindent
* Merge the original v41-0004 tap-test.
* Addressed the comments above.
* Addressed the Shveta's comments[1].

0002:
* ran pgindent

0003:
* ran pgindent

0004:
* ran pgindent

0005:
* ran pgindent

[1] https://www.postgresql.org/message-id/CAJpy0uAg1mTcy00nR%3DVAx1nTJYRkQF84YOY4_YKh8L53A1t6sA%40mail.gmail.com

Best Regards,
Hou zj

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.