On Tue, Aug 23, 2016 at 12:59 PM, Craig Ringer <cr...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers