Re: Conflict detection for update_deleted in logical replication

On Mon, Jul 28, 2025 at 4:38 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Fri, Jul 25, 2025 at 4:38 PM Zhijie Hou (Fujitsu)
> <houzj(dot)fnst(at)fujitsu(dot)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.
>
>
> 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.
>
>
> 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.
>
>
> 4)
> catalogs.sgml:
> + If true, he detection of <xref linkend="conflict-update-deleted"/> is
> he --> the
>

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?

thanks
Shveta