On Friday, August 1, 2025 7:42 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > On Fri, Aug 1, 2025 at 5:02 PM Zhijie Hou (Fujitsu) <houzj.f...@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. [1] https://www.postgresql.org/message-id/CAA4eK1%2B2tZ0rGowwpfmPQA03KdBOaeaK6D5omBN76UTP2EPx6w%40mail.gmail.com [2] https://www.postgresql.org/message-id/CAJpy0uDNhP%2BQeH-zGLBgMnRY1JZGVeoZ_dxff5S6HmpnRcWk8A%40mail.gmail.com Best Regards, Hou zj
v56-0003-Re-create-the-replication-slot-if-the-conflict-r.patch
Description: v56-0003-Re-create-the-replication-slot-if-the-conflict-r.patch
v56-0001-Support-the-conflict-detection-for-update_delete.patch
Description: v56-0001-Support-the-conflict-detection-for-update_delete.patch
v56-0002-Introduce-a-new-GUC-max_conflict_retention_durat.patch
Description: v56-0002-Introduce-a-new-GUC-max_conflict_retention_durat.patch
