Re: Make HeapTupleSatisfiesMVCC more concurrent

On Tue, Aug 18, 2015 at 8:36 PM, 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.

Like Jeff's approach, this will set hint bits less aggressively.
Suppose there are 10 processes concurrently scanning a whole bunch of
dead tuples.  Furthermore, suppose those dead tuples were created by a
transaction that began after those processes took their snapshot and
later aborted.  Without the patch, the first one should notice that
the tuples are in fact dead and hint them HEAP_XMIN_INVALID.  With the
patch, the backends will see those transactions as in-progress rather
than aborted, so they won't be hinted.

Now that probably isn't a reason not to do this.  Anything that cuts
down on ProcArrayLock acquisition and cache-line bouncing in shared
memory is likely to be a win even if it burns a few more cycles
elsewhere.  And I think the scenario I just mentioned probably doesn't
happen that often, and probably wouldn't cost that much if it did.
But it's worth thinking about, and at the least we should document in
the commit message or the comments that this change has this effect.

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