Re: what to revert

On 2016-05-04 00:01:20 +0300, Ants Aasma wrote:
> On Tue, May 3, 2016 at 9:57 PM, Tomas Vondra
> <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
> > If you tell me how to best test it, I do have a 4-socket server sitting idly
> > in the corner (well, a corner reachable by SSH). I can get us some numbers,
> > but I haven't been following the snapshot_too_old so I'll need some guidance
> > on what to test.
>
> I worry about two contention points with the current implementation.
>
> The main one is the locking within MaintainOldSnapshotTimeMapping()
> that gets called every time a snapshot is taken. AFAICS this should
> show up by setting old_snapshot_threshold to any positive value and
> then running a simple within shared buffers scale factor read only
> pgbench at high concurrency (number of CPUs or a small multiple). On a
> single socket system this does not show up.

On a two socket system it does, check the bottom of:
http://archives.postgresql.org/message-id/20160413171955.i53me46fqqhdlztq%40alap3.anarazel.de
Note that this (accidentally) compares thresholds 0 and 10 (instead of
-1 and 10), but that's actually interesting for this question because of
the quick exit in MaintainOldSnapshotData:
/* No further tracking needed for 0 (used for testing). */
if (old_snapshot_threshold == 0)
return;
which happens before the lwlock is taken.

> The second one is probably a bit harder to hit,
> GetOldSnapshotThresholdTimestamp() has a spinlock that gets hit
> everytime a scan sees a page that has been modified after the snapshot
> was taken. A workload that would tickle this is something that uses a
> repeatable read snapshot, builds a non-temporary table and runs
> reporting on it.

I'm not particularly concerned about that kind of issue - we can quite
easily replace that spinlock with 64bit atomic reads/writes for which
I've already proposed a patch. I'd expect that to go into 9.7.

Greetings,

Andres Freund