RE: Conflict detection for update_deleted in logical replication

On Wednesday, January 8, 2025 3:49 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Tue, Jan 7, 2025 at 6:04 PM Zhijie Hou (Fujitsu) <houzj(dot)fnst(at)fujitsu(dot)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