Re: A patch for get origin from commit_ts.

On Tue, Jul 07, 2020 at 10:02:29AM +0800, movead(dot)li(at)highgo(dot)ca wrote:
> Thanks, very helpful information and I have followed that.

Cool, thanks. I have gone through your patch in details, and updated
it as the attached. Here are some comments.

'8000' as OID for the new function was not really random, so to be
fair with the other patches, I picked up the first random value
unused_oids has given me instead.

There were some indentation issues, and pgindent got that fixed.

I think that it is better to use "replication origin" in the docs
instead of just origin. I have kept "origin" in the functions for
now as that sounded cleaner to me, but we may consider using something
like "reporigin" as well as attribute name.

The tests could just use tstzrange() to make sure that the timestamps
have valid values, so I have switched to that, and did not resist to
do the same in the existing tests.

+-- Test when it can not find the transaction
+SELECT * FROM pg_xact_commit_timestamp_origin((:'txid_set_origin'::text::int +
10)::text::xid) x;
This test could become unstable, particularly if it gets used in a
parallel environment, so I have removed it. Perhaps I am just
over-pessimistic here though..

As a side note, I think that we could just remove the alternate output
of commit_ts/, as it does not really get used because of the
NO_INSTALLCHECK present in the module's Makefile. That would be the
job of a different patch, so I have updated it accordingly. Glad to
see that you did not forget to adapt it in your own patch.

(The change in catversion.h is a self-reminder...)
--
Michael