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.
