Re: Conflict detection for update_deleted in logical replication

On Fri, Oct 11, 2024 at 2:04 PM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
>
> Attach the V4 patch set which addressed above comments.
>

A few minor comments:
1.
+ * Retaining the dead tuples for this period is sufficient because any
+ * subsequent transaction from the publisher will have a later timestamp.
+ * Therefore, it is acceptable if dead tuples are removed by vacuum and an
+ * update_missing conflict is detected, as the correct resolution for the
+ * last-update-wins strategy in this case is to convert the UPDATE to an INSERT
+ * and apply it anyway.
+ *
+ * The 'remote_wal_pos' will be reset after sending a new request to walsender.
+ */
+static void
+maybe_advance_nonremovable_xid(XLogRecPtr *remote_wal_pos,
+ DeadTupleRetainPhase *phase)

We should cover the key point of retaining dead tuples which is to
avoid converting updates to inserts (considering the conflict as
update_missing) in the comments above and also in the commit message.

2. In maybe_advance_nonremovable_xid() all three phases are handled by
different if blocks but as per my understanding the phase value will
be unique in one call to the function. So, shouldn't it be handled
with else if?

--
With Regards,
Amit Kapila.