> -----Original Message----- > From: Dilip Kumar <dilipbal...@gmail.com> > Sent: Friday, August 1, 2025 6:29 PM > To: Hou, Zhijie/侯 志杰 <houzj.f...@fujitsu.com> > Cc: Amit Kapila <amit.kapil...@gmail.com>; shveta malik > <shveta.ma...@gmail.com>; Kuroda, Hayato/黒田 隼人 > <kuroda.hay...@fujitsu.com>; pgsql-hackers > <pgsql-hack...@postgresql.org>; vignesh C <vignes...@gmail.com>; Nisha > Moond <nisha.moond...@gmail.com>; Masahiko Sawada > <sawada.m...@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.f...@fujitsu.com> > wrote: > > > > On Thursday, July 31, 2025 5:29 PM Amit Kapila <amit.kapil...@gmail.com> > wrote: > > > > > > On Tue, Jul 29, 2025 at 10:51 AM Zhijie Hou (Fujitsu) > > > <houzj.f...@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
