RE: Conflict detection for update_deleted in logical replication

> -----Original Message-----
> From: Dilip Kumar <dilipbalaut@gmail.com>
> Sent: Friday, August 1, 2025 6:29 PM
> To: Hou, Zhijie/侯 志杰 <houzj.fnst@fujitsu.com>
> Cc: Amit Kapila <amit.kapila16@gmail.com>; shveta malik
> <shveta.malik@gmail.com>; Kuroda, Hayato/黒田 隼人
> <kuroda.hayato@fujitsu.com>; pgsql-hackers
> <pgsql-hackers@postgresql.org>; vignesh C <vignesh21@gmail.com>; Nisha
> Moond <nisha.moond412@gmail.com>; Masahiko Sawada
> <sawada.mshk@gmail.com>
> Subject: Re: Conflict detection for update_deleted in logical replication
> 
> On Thu, Jul 31, 2025 at 3:49 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> wrote:
> >
> > On Thursday, July 31, 2025 5:29 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> > >
> > > On Tue, Jul 29, 2025 at 10:51 AM Zhijie Hou (Fujitsu)
> > > <houzj.fnst@fujitsu.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