Re: Conflict detection for update_deleted in logical replication

On Tue, Dec 3, 2024 at 9:17 AM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> Also, I removed a change in wait_for_local_flush() which was mis-added in V13_2
> patch.
>

1.
+ if (can_advance_nonremovable_xid(&data, last_recv_timestamp))
+ maybe_advance_nonremovable_xid(&data);

In can_advance_nonremovable_xid(), we determine whether to advance the
oldest xid based on 'last_recv_timestamp' and then again in
maybe_advance_nonremovable_xid()->get_candidate_xid(), we compare it
with the current time. How does that make sense? Shall we use
'last_recv_timestamp' directly in get_candidate_xid() as that will
avoid the additional time check in can_advance_nonremovable_xid()?

2.
+ TimestampTz next_attempt_time; /* when to attemp to advance the xid during
+ * change application */
+} RetainConflictInfoData;

This new variable introduced in this version is not used in the patch.
Any reason or just a leftover?

Apart from the above, I have made a few updates in the comments in the
attached. Please include those after review.

--
With Regards,
Amit Kapila.