Re: Conflict Detection and Resolution

Hello hackers,

Hey I'm Diego and I do work for Percona and started to work on PostgreSQL
and I would like to contribute to the project moving forward.

I have been following this thread since the beginning, but due to my
limited knowledge of the overall code structure, my first review of the
provided patches was more focused on validating the logic and general flow.

I have been testing the provided patches and so far the only issue I have
is the one reported about DirtySnapshot scans over a B-tree with parallel
updates, which may skip/not find some records.

That said, I'd like to know if it's worthwhile pulling the proposed fix on
[0] and validating/updating the code to fix the issue or if there are other
better solutions being discussed?

Thanks for your attention,
Diego

[0]:
https://www.postgresql.org/message-id/flat/CANtu0oiziTBM8+WDtkktMZv0rhGBroYGWwqSQW+MzOWpmk-XEw(at)mail(dot)gmail(dot)com#74f5f05594bb6f10b1d882a1ebce377c

On Mon, Oct 21, 2024 at 2:04 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:

> On Fri, Oct 18, 2024 at 4:30 PM Zhijie Hou (Fujitsu)
> <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> >
> > On Wednesday, October 9, 2024 2:34 PM shveta malik <
> shveta(dot)malik(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Oct 9, 2024 at 8:58 AM shveta malik <shveta(dot)malik(at)gmail(dot)com>
> > > wrote:
> > > >
> > > > On Tue, Oct 8, 2024 at 3:12 PM Nisha Moond
> > > <nisha(dot)moond412(at)gmail(dot)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?
> >
> > I think simply reporting a warning and applying remote changes without
> further
> > action could lead to data inconsistencies between nodes. Considering the
> > potential challenges and time required to recover from these
> inconsistencies, I
> > prefer to keep reporting errors, in which case users have an opportunity
> to
> > resolve the issue by enabling track_commit_timestamp.
> >
>
> Okay, makes sense. We should raise ERROR then.
>
> thanks
> Shveta
>
>
>