RE: Conflict detection for update_deleted in logical replication

On Monday, July 28, 2025 7:43 PM shveta malik <shveta.malik@gmail.com> wrote:
> On Mon, Jul 28, 2025 at 4:38 PM shveta malik <shveta.malik@gmail.com>
> wrote:
> >
> > On Fri, Jul 25, 2025 at 4:38 PM Zhijie Hou (Fujitsu)
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > >
> > > The V53-0001 also includes Shveta's comments in [1].
> > >
> >
> > Thanks, I have not yet completed the review, but please find a few
> > comments on 001:
> >
> > 1)
> > IsIndexUsableForFindingDeletedTuple()
> > We first have:
> > + /*
> > + * A frozen transaction ID indicates that it must fall behind the
> > + conflict
> > + * detection slot.xmin.
> > + */
> > + if (HeapTupleHeaderXminFrozen(index_tuple->t_data))
> > + return true;
> >
> > thent his:
> > + index_xmin = HeapTupleHeaderGetRawXmin(index_tuple->t_data);
> >
> > Shall we use HeapTupleHeaderGetXmin() instead of above 2? We can check
> > if xid returned by HeapTupleHeaderGetXmin() is FrozenTransactionId or
> > normal one and then do further processing.

I have simplified the code to avoid unnecessary checks and added generic
error handling and proper resource release, which was overlooked in the
previous version.

> >
> >
> > 2)
> > Both FindRecentlyDeletedTupleInfoByIndex and
> > FindRecentlyDeletedTupleInfoSeq() has:
> > + /* Exit early if the commit timestamp data is not available */ if
> > + (!track_commit_timestamp) return false;
> >
> > We shall either move this check to FindDeletedTupleInLocalRel() or in
> > the caller of FindDeletedTupleInLocalRel() where we check
> > 'MySubscription->retaindeadtuples'. Moving to the caller of
> > FindDeletedTupleInLocalRel() looks better as there is no need to call
> > FindDeletedTupleInLocalRel itself if pre-conditions are not met.

Moved.

> >
> >
> > 3)
> > FindRecentlyDeletedTupleInfoSeq():
> > + * during change applications. While this approach may be slow on
> > + large tables,
> > + * it is considered acceptable because it is only used in rare
> > + conflict cases
> > + * where the target row for an update cannot be found.
> > + */
> > Shall we extend the last line to:
> > where the target row for an update cannot be found and index scan can
> > not be used.

Changed.

> >
> >
> > 4)
> > catalogs.sgml:
> > +       If true, he detection of <xref
> > + linkend="conflict-update-deleted"/> is
> > he --> the
> >

Fixed.

> 
> 5)
> FindRecentlyDeletedTupleInfoSeq():
> 
> + /* Get the cutoff xmin for HeapTupleSatisfiesVacuum */ oldestxmin =
> + GetOldestNonRemovableTransactionId(rel);
> 
> Another point is which xid should be used as threshold in
> HeapTupleSatisfiesVacuum() to decide if tuple is DEAD or RECENTLY-DEAD
> for update_deleted case? Currently we are using
> GetOldestNonRemovableTransactionId() but the xid returned here could be
> older than pg_conflict_detection's xmin in presence of other logical slots which
> have older effective_xmin. Shall we use pg_conflict_detection's xmin instead
> as threshold or worker's oldest_nonremovable_xid?

I thought both approaches are fine in terms of correctness, because even if the
apply worker will use the outdated xmin to determine the recently dead tuples,
user can still compare the timestamp to choose the correct resolution.

OTOH, I agree that it would be better to use a stricter rule, so I changed to
use the apply worker's oldest_nonremoable_xid to determine recently dead tuples.

To be consistent, I also used this xid instead of conflict detection slot.xmin to
determine whether an index is usable or not.

This is the V54 patch set, with only patch 0001 updated to address the latest
comments.

Patch 0002 includes separate code for statistics but must be applied together
with 0001 to pass the regression tests. This is because the current
implementation assumes the number of conflict type in statistics matches the
general ConflictType enum; otherwise, it may access an invalid memory area
during stat collection.

Best Regards,
Hou zj