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

On Tue, Aug 23, 2016 at 12:59 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> Also fine by me. You're right, keep it simple. It means the potential set of
> values isn't discoverable the same way, but ... meh. Using it usefully means
> reading the docs anyway.
>
> The remaining 2 patches of interest are attached - txid_status() and
> txid_convert_if_recent(). Thanks for committing txid_current_if_assigned().
>
> Now I'd best stop pretending I'm in a sensible timezone.

I reviewed this version some more and found some more problems.

+ uint32 xid_epoch = (uint32)(xid_with_epoch >>32);
+ TransactionId xid = (TransactionId)(xid_with_epoch);

I think this is not project style. In particular, I think that the
first one needs a space after the cast and another space before the
32; and I think the second one has an unnecessary set of parentheses
and needs a space added.

+/*
+ * Underlying implementation of txid_status, which is mapped to an enum in
+ * system_views.sql.
+ */

Not any more...

+ errmsg("transaction ID "UINT64_FORMAT" is an invalid xid",
+ xid_with_epoch)));

Spacing.

+ if (TransactionIdIsCurrentTransactionId(xid) ||
TransactionIdIsInProgress(xid))
+ status = gettext_noop("in progress");
+ else if (TransactionIdDidCommit(xid))
+ status = gettext_noop("committed");
+ else if (TransactionIdDidAbort(xid))
+ status = gettext_noop("aborted");
+ else
+ /* shouldn't happen */
+ ereport(ERROR,
+ (errmsg_internal("unable to determine commit status of
xid "UINT64_FORMAT, xid)));

Maybe I'm all wet here, but it seems like there might be a problem
here if the XID is older than the CLOG truncation point but less than
one epoch old. get_xid_in_recent_past only guarantees that the
transaction is less than one epoch old, not that we still have CLOG
data for it. And there's nothing to keep NextXID from advancing under
us, so if somebody asks about a transaction that's just under 2^32
transactions old, then get_xid_in_recent_past could say it's valid,
then NextXID advances and we look up the XID extracted from the txid
and get the status of the new transaction instead of the old one!

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