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