On Tue, Jan 7, 2025 at 6:04 PM Zhijie Hou (Fujitsu) <houzj.f...@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'. ``` postgres=# show track_commit_timestamp; track_commit_timestamp ------------------------ off (1 row) postgres=# select subname, subretainconflictinfo from pg_subscription; subname | subretainconflictinfo ---------+----------------------- sub21 | t sub22 | t ``` 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". -- Thanks, Nisha
