Re: Conflict detection and logging in logical replication

On Fri, Jul 26, 2024 at 3:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Jul 26, 2024 at 3:37 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Fri, Jul 26, 2024 at 3:03 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> > >
> > > On Thu, Jul 25, 2024 at 12:04 PM Zhijie Hou (Fujitsu)
> > > <houzj.fnst@fujitsu.com> wrote:
> > > > Here is the V6 patch set which addressed Shveta and Nisha's comments
> > > > in [1][2][3][4].
> > >
> > > Thanks for the patch.
> > > I tested the v6-0001 patch with partition table scenarios. Please
> > > review the following scenario where Pub updates a tuple, causing it to
> > > move from one partition to another on Sub.
> > >
> > > Setup:
> > > Pub:
> > >  create table tab (a int not null, b int not null);
> > >  alter table tab add constraint tab_pk primary key (a,b);
> > > Sub:
> > >  create table tab (a int not null, b int not null) partition by range (b);
> > >  alter table tab add constraint tab_pk primary key (a,b);
> > >  create table tab_1 partition of tab FOR values from (MINVALUE) TO (100);
> > >  create table tab_2 partition of tab FOR values from (101) TO (MAXVALUE);
> > >
> > > Test:
> > >  Pub: insert into tab values (1,1);
> > >  Sub: update tab set a=1 where a=1;  > just to make it Sub's origin
> > >  Sub: insert into tab values (1,101);
> > >  Pub: update b=101 where b=1; --> Both 'update_differ' and
> > > 'insert_exists' are detected.
> > >
> > > For non-partitioned tables, a similar update results in
> > > 'update_differ' and 'update_exists' conflicts. After detecting
> > > 'update_differ', the apply worker proceeds to apply the remote update
> > > and if a tuple with the updated key already exists, it raises
> > > 'update_exists'.
> > > This same behavior is expected for partitioned tables too.
> >
> > Good catch. Yes, from the user's perspective, an update_* conflict
> > should be raised when performing an update operation. But internally
> > since we are deleting from one partition and inserting to another, we
> > are hitting insert_exist. To convert this insert_exist to udpate_exist
> > conflict, perhaps we need to change insert-operation to
> > update-operation as the default resolver is 'always apply update' in
> > case of update_differ.
> >
>
> But we already document that behind the scenes such an update is a
> DELETE+INSERT operation [1]. Also, all the privilege checks or before
> row triggers are of type insert, so, I think it is okay to consider
> this as insert_exists conflict and document it. Later, resolver should
> also fire for insert_exists conflict.

Thanks for the link. +1 on  existing behaviour of insert_exists conflict.

> 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. 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'. Thoughts?

thanks
Shveta