On Wed, Aug 19, 2015 at 6:06 AM, Tom Lane <t...@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. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com