RE: Conflict detection for update_deleted in logical replication

> -----Original Message-----
> From: Dilip Kumar <dilipbalaut(at)gmail(dot)com>
> Sent: Friday, August 1, 2025 6:29 PM
> To: Hou, Zhijie/侯 志杰 <houzj(dot)fnst(at)fujitsu(dot)com>
> Cc: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>; shveta malik
> <shveta(dot)malik(at)gmail(dot)com>; Kuroda, Hayato/黒田 隼人
> <kuroda(dot)hayato(at)fujitsu(dot)com>; pgsql-hackers
> <pgsql-hackers(at)postgresql(dot)org>; vignesh C <vignesh21(at)gmail(dot)com>; Nisha
> Moond <nisha(dot)moond412(at)gmail(dot)com>; Masahiko Sawada
> <sawada(dot)mshk(at)gmail(dot)com>
> Subject: Re: Conflict detection for update_deleted in logical replication
>
> On Thu, Jul 31, 2025 at 3:49 PM Zhijie Hou (Fujitsu) <houzj(dot)fnst(at)fujitsu(dot)com>
> wrote:
> >
> > On Thursday, July 31, 2025 5:29 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> > >
> > > On Tue, Jul 29, 2025 at 10:51 AM Zhijie Hou (Fujitsu)
> > > <houzj(dot)fnst(at)fujitsu(dot)com>
> > > wrote:
> > > >
> > > > This is the V54 patch set, with only patch 0001 updated to address
> > > > the latest comments.
> > > >
> > >
> > > Few minor comments:
> >
> > Thanks for the comments.
> >
> > > 1.
> > > /* The row to be updated was deleted by a different origin */
> > > CT_UPDATE_DELETED,
> > > /* The row to be updated was modified by a different origin */
> > > CT_UPDATE_ORIGIN_DIFFERS,
> > > /* The updated row value violates unique constraint */
> > > CT_UPDATE_EXISTS,
> > > /* The row to be updated is missing */ CT_UPDATE_MISSING,
> > >
> > > Is there a reason to keep CT_UPDATE_DELETED before
> > > CT_UPDATE_ORIGIN_DIFFERS? I mean why not keep it just before
> > > CT_UPDATE_MISSING on the grounds that they are always handled
> together?
> >
> > I agree that it makes more sense to put it before update_missing, and
> changed it.
> >
> > >
> > > 2. Will it be better to name FindRecentlyDeletedTupleInfoByIndex as
> > > RelationFindDeletedTupleInfoByIndex to make it similar to existing
> > > function RelationFindReplTupleByIndex? If you agree then make a
> > > similar change for FindRecentlyDeletedTupleInfoSeq as well.
> >
> > Yes, the suggested name looks better.
> >
> > >
> > > Apart from above, please find a number of comment edits and other
> > > cosmetic changes in the attached.
> >
> > Thanks, I have addressed above comments and merge the patch into 0001.
>
> I have few comments in 0001
>

Thanks for the comments!

>
> 2.
>
> + If set to <literal>true</literal>, the detection of
> + <xref linkend="conflict-update-deleted"/> is enabled, and a
> physical
> + replication slot named
> <quote><literal>pg_conflict_detection</literal></quote>
> created on the subscriber to prevent the conflict information from
> being removed.
>
> "to prevent the conflict information from being removed." should be rewritten
> as "to prevent removal of tuple required for conflict detection"

It appears the document you commented is already committed. I think the
intention was to make a general statement that neither dead tuples nor commit
timestamp data would be removed.

>
> 3.
> + /* Return if the commit timestamp data is not available */
> + if (!track_commit_timestamp)
> + return false;
>
> Shouldn't caller should take care of this? I mean if the 'retaindeadtuples' and
> 'track_commit_timestamp' is not set then caller shouldn't even call this
> function.

I feel moving the checks into a single central function would streamline the
caller, reducing code duplication. So, maybe we could move the retaindeadtuple
check into this function as well for consistency. Thoughts ?

Best Regards,
Hou zj