Re: Conflict detection for update_deleted in logical replication

On Fri, Aug 1, 2025 at 9:16 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Friday, August 1, 2025 7:42 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Fri, Aug 1, 2025 at 5:02 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> > wrote:
> > >
> > > > 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.
> >
> > Okay got it, so instead of "conflict information" should we say "information for
> > detecting conflicts" or "conflict detection information", conflict information
> > looks like we want to prevent the information about the conflict which has
> > already happened, instead we are preventing information which are required
> > for detecting the conflict, does this make sense?
>
> It makes sense to me, so changed.
>
> >
> > I know this is already committed, but actually this is part of the whole patch set
> > so we can always improvise it.
> >
> > > >
> > > > 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 ?
> >
> > Fine with either way, actually I wanted both the check 'retaindeadtuple' and
> > 'track_commit_timestamp' at the same place.
>
> Thanks for confirming. Here is V56 patch set which addressed all the
> comments including the comments from Amit[1] and Shveta[2].
>
> I have merged V55-0002 into 0001 and updated the list of author
> and reviewers based on my knowledge.

Now this LGTM, I would suggest some modification in the commit
message, I think the purpose of this improvement is not very clear to
me, it says "supports detecting update_deleted conflicts during update
operations". But I think we can be more clearer on when we want to
detect such conflicts, here is my proposal, feel free to edit/revise/

"This commit improves conflict detection for updates. When an update
arrives for a row that has been locally deleted, the subscriber now
performs a secondary scan to check for the recently deleted tuple.
This check uses the old column values from remote tuple to find
a match among tuples that are visible to 'SnapshotAny' but not yet
vacuumed away.

If a matching, deleted tuple is found, an update_deleted conflict is
raised.  This provides a more robust and accurate conflict resolution
process, preventing unexpected behavior by correctly identifying cases
where a remote update clashes with a local deletion."

-- 
Regards,
Dilip Kumar
Google