Re: [PATCH] Transaction traceability - txid_status(bigint)

On Mon, Aug 22, 2016 at 8:55 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> Updated patch series attached. As before, 0-4 intended for commit, 5 just
> because it'll be handy to have around for people doing wraparound related
> testing.
>
> Again, thanks for taking a look.

/me reviews a bit more deeply.

In 0001, it seems to me that "in-progress" should be "in progress". I
don't think it's normal to hyphenate that. We have admittedly
sometimes done so, but:

[rhaas pgsql]$ git grep 'in-progress' | wc -l
63
[rhaas pgsql]$ git grep 'in progress' | wc -l
346

It may make sense to speak of an "in-progress transaction" but I would
say "the transaction is in progress" not "the transaction is
in-progress", which seems to me to argue for a space as the proper
separator here.

Also:

+CREATE TYPE txid_status AS ENUM ('committed', 'in-progress', 'aborted');
+
+CREATE FUNCTION
+ txid_status(txid bigint)
+RETURNS txid_status
+LANGUAGE sql
+VOLATILE PARALLEL SAFE
+AS $$
+SELECT CASE
+ WHEN s IS NULL THEN NULL::txid_status
+ WHEN s = -1 THEN 'aborted'::txid_status
+ WHEN s = 0 THEN 'in-progress'::txid_status
+ WHEN s = 1 THEN 'committed'::txid_status
+END
+FROM pg_catalog.txid_status_internal($1) s;
+$$;
+
+COMMENT ON FUNCTION txid_status(bigint)
+IS 'get commit status of given recent xid or null if too old';

I'm not really that keen on this approach. I don't think we need to
introduce a new data type for this, and I would rather not use SQL,
either. It would be faster and simpler to just return the appropriate
string from a C function defined directly.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company