On Mon, Jun 29, 2020 at 06:17:27PM -0700, Madan Kumar wrote: > We already have pg_xact_commit_timestamp() that returns the timestamp of > the commit. It may be better to have one single function returning both > timestamp and origin for a given transaction ID. > > A second thing is that TransactionIdGetCommitTsData() was introdued in > core(73c986add). It has only one caller pg_xact_commit_timestamp() which > passes RepOriginId as NULL, making last argument to the > TransactionIdGetCommitTsData() a dead code in core. > > Quick code search shows that it is getting used by pglogical (caller: > https://sources.debian.org/src/pglogical/2.3.2-1/pglogical_conflict.c/?hl=509#L509). > CCing Craig Ringer and Petr Jelinek for the inputs.
Another question that has popped up when doing this review is what would be the use-case of adding this information at SQL level knowing that logical replication exists since 10? Having dead code in the backend tree is not a good idea of course, so we can also have as argument to simplify TransactionIdGetCommitTsData(). Now, pglogical has pglogical_xact_commit_timestamp_origin() to get the replication origin with its own function so providing an extending equivalent returning one row with two fields would be nice for pglogical so as this function is not necessary. As mentioned by Madan, the portion of the code using TransactionIdGetCommitTsData() relies on it for conflicts of updates (the first win, last win logic at quick glance). I am adding Peter E in CC for an opinion, the last commits of pglogical are from him. -- Michael
signature.asc
Description: PGP signature
