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

1.
+   /* Select the dead tuple with the most recent commit timestamp */
+   if (TransactionIdGetCommitTsData(xmax, &localts, &localorigin) &&
+       (TimestampDifferenceExceeds(*delete_time, localts, 0) ||
+        *delete_time == 0))

IMHO the "*delete_time == 0" is a redundant check, because if
*delete_time is 0 then TimestampDifferenceExceeds will always be true
as localts can not be 0.

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"

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.

4.
+   /*
+    * Instead of invoking GetOldestNonRemovableTransactionId() for conflict
+    * detection, we use the conflict detection slot.xmin. This value will be
+    * greater than or equal to the other threshold and provides a more direct
+    * and efficient way to identify recently deleted dead tuples relevant to
+    * the conflict detection. The oldest_nonremovable_xid is not used here,
+    * as it is maintained only by the leader apply worker and unavailable to
+    * table sync and parallel apply workers.
+    */
+   slot = SearchNamedReplicationSlot(CONFLICT_DETECTION_SLOT, true);

This comment seems a bit confusing to me, Isn't it actually correct to
just use the "conflict detection slot.xmin" even without any other
reasoning?


-- 
Regards,
Dilip Kumar
Google