On Wed, Aug 19, 2015 at 6:06 AM, Tom Lane <
tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > Just thinking about this ... I wonder why we need to call
> > TransactionIdIsInProgress() at all rather than believing the answer from
> > the snapshot? Under what circumstances could TransactionIdIsInProgress()
> > return true where XidInMVCCSnapshot() had not?
>
> I experimented with the attached patch, which replaces
> HeapTupleSatisfiesMVCC's calls of TransactionIdIsInProgress with
> XidInMVCCSnapshot, and then as a cross-check has all the "return false"
> exits from XidInMVCCSnapshot assert !TransactionIdIsInProgress().
> The asserts did not fire in the standard regression tests nor in a
> pgbench run, which is surely not proof of anything but it suggests
> that I'm not totally nuts.
>
> I wouldn't commit the changes in XidInMVCCSnapshot for real, but
> otherwise this is a possibly committable patch.
>
I think one case where the patch can impact is for aborted transactions.
In TransactionIdIsInProgress(), we check for aborted transactions before
consulting pg_subtrans whereas with patch it will consult pg_subtrans
without aborted transaction check. Now it could be better to first check
pg_subtrans if we found that the corresponding top transaction is
in-progress as that will save extra pg_clog lookup, but I just mentioned
because that is one difference that I could see with this patch.
Another minor point is, I think we should modify function level comment
for XidInMVCCSnapshot() where it says that this applies to known-
committed XIDs which will no longer be true after this patch.
Apart from above, the patch looks good.
