On Wed, Mar 22, 2017 at 3:13 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> Changes made per discussion.
>
> This looks good to me also. Does what we need it to do.
>
> I was a little worried by possible performance of new lock, but its
> not intended to be run frequently, so its OK.
Agreed.
Reviewing 0002:
+ if (!TransactionIdIsValid(xid))
+ {
+ LWLockRelease(XidGenLock);
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("transaction ID " UINT64_FORMAT " is an invalid xid",
+ xid_with_epoch)));
+ }
It's unnecessary to release LWLockRelease() before throwing an error,
and it's also wrong because we haven't acquired XidGenLock in this
code path. But maybe it would be better to just remove this entirely
and instead have TransactionIdInRecentPast return false for
InvalidTransactionId. Then we'd avoid adding a translatable message
for a case that is basically harmless to allow.
+ if (TransactionIdIsCurrentTransactionId(xid))
+ status = gettext_noop("in progress");
+ else if (TransactionIdDidCommit(xid))
+ status = gettext_noop("committed");
+ else if (TransactionIdDidAbort(xid))
+ status = gettext_noop("aborted");
+ else
+
+ /*
+ * can't test TransactionIdIsInProgress here or we race with
+ * concurrent commit/abort. There's no point anyway, since it
+ * might then commit/abort just after we check.
+ */
+ status = gettext_noop("in progress");
I am not sure this is going to do the right thing for transactions
that are aborted by a crash without managing to write an abort record.
It seems that the first check will say the transaction isn't in
progress, and the second and third checks will say it isn't either
committed or aborted since, if I am reading this correctly, they just
read clog directly. Compare HeapTupleSatisfiesMVCC, which assumes
that a not-in-progress, not-committed transaction must necessarily
have aborted. I think your comment is pointing to a real issue,
though. It seems like what might be needed is to add one more check.
Before where you have the "else" clause, check if the XID is old, e.g.
precedes our snapshot's xmin. If so, it must be committed or aborted
and, since it didn't commit, it aborted. If not, it must've changed
from in progress to not-in-progress just as we were in the midst
checking, so labeling it as in progress is fine.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
