On Mon, Jul 28, 2025 at 4:38 PM shveta malik <shveta.ma...@gmail.com> wrote: > > On Fri, Jul 25, 2025 at 4:38 PM Zhijie Hou (Fujitsu) > <houzj.f...@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. > > > 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
