On Wed, Oct 9, 2024 at 8:58 AM shveta malik <shveta.ma...@gmail.com> wrote: > > On Tue, Oct 8, 2024 at 3:12 PM Nisha Moond <nisha.moond...@gmail.com> wrote: > > >
Please find few comments on v14-patch004: patch004: 1) GetConflictResolver currently errors out when the resolver is last_update_wins and track_commit_timestamp is disabled. It means every conflict resolution with this resolver will keep on erroring out. I am not sure if we should emit ERROR here. We do emit ERROR when someone tries to configure last_update_wins but track_commit_timestamp is disabled. I think that should suffice. The one in GetConflictResolver can be converted to WARNING max. What could be the side-effect if we do not emit error here? In such a case, the local timestamp will be 0 and remote change will always win. Is that right? If so, then if needed, we can emit a warning saying something like: 'track_commit_timestamp is disabled and thus remote change is applied always.' Thoughts? 2) execReplication.c: There are some optimizations in this file (moving duplicate code to has_conflicting_tuple), I think these optimizations are applicable even to patch003 (or patch002 as well?) and thus can be moved there. Please review once. thanks Shveta
