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: Nisha Moond <nisha.moond412@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Masahiko Sawada <sawada.mshk@gmail.com>, shveta malik <shveta.malik@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2025-01-09T09:15:44Z
Lists: pgsql-hackers 
On Wednesday, January 8, 2025 3:49 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> 
> On Tue, Jan 7, 2025 at 6:04 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> wrote:
> >
> >
> > Attached the V19 patch which addressed comments in [1][2][3][4][5][6][7].
> >
> 
> Here are a couple of initial review comments on v19 patch set:
> 
> 1) The subscription option 'retain_conflict_info' remains set to "true" for a
> subscription even after restarting the server with
> 'track_commit_timestamp=off', which can lead to incorrect behavior.
>   Steps to reproduce:
>    1. Start the server with 'track_commit_timestamp=ON'.
>    2. Create a subscription with (retain_conflict_info=ON).
>    3. Restart the server with 'track_commit_timestamp=OFF'.
> 
>  - The apply worker starts successfully, and the subscription retains
> 'retain_conflict_info=true'. However, in this scenario, the update_deleted
> conflict detection will not function correctly without
> 'track_commit_timestamp'.
> ```

IIUC, track_commit_timestamp is a GUC that designed mainly for conflict
detection, so it seems an unreasonable behavior to me if user enable this when
creating the sub but disable is afterwards. Besides, we documented that
update_deleted conflict would not be detected when track_commit_timestamp is
not enabled, so I am not sure if it's worth more effort adding checks for this
case.

> 
> 2) With the new parameter name change to "retain_conflict_info", the error
> message for both the 'CREATE SUBSCRIPTION' and 'ALTER SUBSCRIPTION'
> commands needs to be updated accordingly.
> 
>   postgres=# create subscription sub11 connection 'dbname=postgres'
> publication pub1 with (retain_conflict_info=on);
>   ERROR:  detecting update_deleted conflicts requires
> "track_commit_timestamp" to be enabled
>   postgres=# alter subscription sub12 set (retain_conflict_info=on);
>   ERROR:  detecting update_deleted conflicts requires
> "track_commit_timestamp" to be enabled
> 
>  - Change the message to something similar - "retaining conflict info requires
> "track_commit_timestamp" to be enabled".

After thinking more, I changed this to a warning for now, because to detect
all necessary conflicts, user must enable the option anyway, and the same has
been documented for update/delete_origin_differs conflicts as well.

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.