Re: Conflict detection and logging in logical replication

On Fri, Jul 26, 2024 at 4:28 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Fri, Jul 26, 2024 at 3:56 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
>
> > One more thing we need to consider is whether we should LOG or ERROR
> > for update/delete_differ conflicts. If we LOG as the patch is doing
> > then we are intentionally overwriting the row when the user may not
> > expect it. OTOH, without a patch anyway we are overwriting, so there
> > is an argument that logging by default is what the user will expect.
> > What do you think?
>
> I was under the impression that in this patch we do not intend to
> change behaviour of HEAD and thus only LOG the conflict wherever
> possible.
>

Earlier, I thought it was good to keep LOGGING the conflict where
there is no chance of wrong data update but for cases where we can do
something wrong, it is better to ERROR out. For example, for an
update_differ case where the apply worker can overwrite the data from
a different origin, it is better to ERROR out. I thought this case was
comparable to an existing ERROR case like a unique constraint
violation. But I see your point as well that one might expect the
existing behavior where we are silently overwriting the different
origin data. The one idea to address this concern is to suggest users
set the detect_conflict subscription option as off but I guess that
would make this feature unusable for users who don't want to ERROR out
for different origin update cases.

> And in the next patch of resolver, based on the user's input
> of error/skip/or resolve, we take the action. I still think it is
> better to stick to the said behaviour. Only if we commit the resolver
> patch in the same version where we commit the detection patch, then we
> can take the risk of changing this default behaviour to 'always
> error'. Otherwise users will be left with conflicts arising but no
> automatic way to resolve those. But for users who really want their
> application to error out, we can provide an additional GUC in this
> patch itself which changes the behaviour to 'always ERROR on
> conflict'.
>

I don't see a need of GUC here, even if we want we can have a
subscription option such conflict_log_level. But users may want to
either LOG or ERROR based on conflict type. For example, there won't
be any data inconsistency in two node replication for delete_missing
case as one is trying to delete already deleted data, so LOGGING such
a case should be sufficient whereas update_differ could lead to
different data on two nodes, so the user may want to ERROR out in such
a case.

We can keep the current behavior as default for the purpose of
conflict detection but can have a separate patch to decide whether to
LOG/ERROR based on conflict_type.

--
With Regards,
Amit Kapila.