Re: Conflict detection and logging in logical replication

On Mon, Aug 19, 2024 at 12:09 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Aug 19, 2024 at 11:54 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
> > On Mon, Aug 19, 2024 at 11:37 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Mon, Aug 19, 2024 at 9:08 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> > > >
> > > > On Sun, Aug 18, 2024 at 2:27 PM Zhijie Hou (Fujitsu)
> > > > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > > > >
> > > > > Attach the V16 patch which addressed the comments we agreed on.
> > > > > I will add a doc patch to explain the log format after the 0001 is RFC.
> > > > >
> > > >
> > > > Thank You for addressing comments. Please see this scenario:
> > > >
> > > > create table tab1(pk int primary key, val1 int unique, val2 int);
> > > >
> > > > pub: insert into tab1 values(1,1,1);
> > > > sub: insert into tab1 values(2,2,3);
> > > > pub: update tab1 set val1=2 where pk=1;
> > > >
> > > > Wrong 'replica identity' column logged? shouldn't it be pk?
> > > >
> > > > ERROR: conflict detected on relation "public.tab1": conflict=update_exists
> > > > DETAIL: Key already exists in unique index "tab1_val1_key", modified
> > > > locally in transaction 801 at 2024-08-19 08:50:47.974815+05:30.
> > > > Key (val1)=(2); existing local tuple (2, 2, 3); remote tuple (1, 2,
> > > > 1); replica identity (val1)=(1).
> > > >
> > >
> > > The docs say that by default replica identity is primary_key [1] (see
> > > REPLICA IDENTITY),
> >
> > yes, I agree. But here the importance of dumping it was to know the
> > value of RI as well which is being used as an identification of row
> > being updated rather than row being conflicted. Value is logged
> > correctly.
> >
>
> Agreed, sorry, I misunderstood the problem reported.

no problem.

>I thought the
> suggestion was to use 'primary key' instead of 'replica identity' but
> you pointed out that the column used in 'replica identity' was wrong.
> We should fix this one.
>

Yes, that is what I pointed out. But let me remind you that this logic
to display both 'Key' and 'RI' is done in the latest patch. Earlier
either 'Key' or 'RI' was logged. But since for 'update_exists', both
make sense, thus I suggested to dump both. 'RI' column is dumped
correctly in all other cases, except this new one. So if fixing this
wrong column name for update_exists adds more complexity, then I am
okay with skipping the 'RI' dump for this case. We’re fine with just
'Key' for now, and we can address this later.

thanks
Shveta