On Tue, May 20, 2025 at 6:30 PM shveta malik wrote: > > Few more comments mostly for patch001:
Thanks for the comments! > > 4) > For this feature, since we are only interested in remote UPDATEs happening > concurrently, so shall we ask primary to provide oldest "UPDATE" > transaction-id in commit-phase rather than any operation's transaction-id? > This may avoid unnecessarily waiting and pinging at subscriber's end in order > to advance oldest_nonremovable-xid. > Thoughts? It is possible, but considering the potential complexity/cost to track UPDATE operations in top-level and sub-transactions, coupled with its limited benefit for workloads featuring frequent UPDATEs on publishers such as observed during TPC-B performance tests, I have opted to document this possibility in comments instead of implementing it in the patch set. > > 5) > + > +/* > + * GetOldestTransactionIdInCommit() > + * > + * Similar to GetOldestActiveTransactionId but returns the oldest > transaction ID > + * that is currently in the commit phase. > + */ > +TransactionId > +GetOldestTransactionIdInCommit(void) > > If there is no transaction currently in 'commit' phase, this function will > then > return the next transaction-id. Please mention this in the comments. I think > the > doc 'protocol-replication.html' should also be updated for the same. I added this info in the doc. But since we have merged this function with GetOldestActiveTransactionId() which has the same behavior, so I am not adding more code comments for the existing function. Best Regards, Hou zj
