Re: Conflict detection for update_deleted in logical replication

On Thu, May 22, 2025 at 8:28 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> Attaching the V31 patch set which addressed comments in [1]~[8].
>

Few comments:
1.
<para>
+             The oldest transaction ID along that is currently in the commit
+             phase on the server, along with its epoch.

The first 'along' in the sentence looks redundant. I've removed this
in the attached.

2.
+ data.remote_oldestxid = FullTransactionIdFromU64(pq_getmsgint64(&s));
+ data.remote_nextxid = FullTransactionIdFromU64(pq_getmsgint64(&s));

Shouldn't we need to typecast the result of pq_getmsgint64(&s) with
uint64 as we do at similar other places in pg_snapshot_recv?

3.

+ pq_sendint64(&output_message,
U64FromFullTransactionId(fullOldestXidInCommit));
+ pq_sendint64(&output_message, U64FromFullTransactionId(nextFullXid));

Similarly, here also we should typecase with uint64

4.
+ * XXX In phase RCI_REQUEST_PUBLISHER_STATUS, a potential enhancement could be
+ * requesting transaction information specifically for those containing
+ * UPDATEs. However, this approach introduces additional complexities in
+ * tracking UPDATEs for transactions on the publisher, and it may not
+ * effectively address scenarios with frequent UPDATEs.

I think, as the patch needs the oldest_nonremovable_xid idea even to
detect update_origin_differs and delete_origin_differs reliably, as
mentioned in 0001's commit message, is it sufficient to track update
transactions? Don't we need to track it even for deletes? I have
removed this note for now and updated the comment to mention it is
required to detect update_origin_differs and delete_origin_differs
conflicts reliably.

Apart from the above comments, I made a few other cosmetic changes in
the attached.

--
With Regards,
Amit Kapila.