Re: [COMMITTERS] pgsql: Add the "snapshot too old" feature

On Mon, Apr 11, 2016 at 4:27 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> Alexander Korotkov wrote:
> > Kevin,
> >
> > This commit makes me very uneasy. I didn't much care about this patch
> > mainly because I didn't imagine its consequences. Now, I see following:
> >
> > 1) We uglify buffer manager interface a lot. This patch adds 3 more
> > arguments to BufferGetPage(). It looks weird. Should we try to find
> less
> > invasive way for doing this?
>
> Kevin's patch was much less invasive originally. It became invasive in
> the course of later review -- there was fear that future code would
> "forget" the snapshot check accidentally, which would have disastrous
> effects (data becomes invisible without notice).
>

OK, I will read that thread and try to verify these thoughts.

> > 2) Did you ever try to examine performance regression? I tried simple
> read
> > only test on 4x18 Intel server.
> > pgbench -s 1000 -c 140 -j 100 -M prepared -S -T 300 -P 1 postgres (data
> > fits to shared_buffers)
> >
> > master - 193 740.3 TPS
> > snapshot too old reverted - 1 065 732.6 TPS
> >
> > So, for read-only benchmark this patch introduces more than 5 times
> > regression on big machine.
>
> Wow, this is terrible, but the patch is supposed to have no impact when
> the feature is not in use. Maybe there's some simple oversight that can
> be fixed.

Perf show that 50% of time is spent in s_lock() called from
GetXLogInsertRecPtr() called from GetSnapshotData(). I think at very least
we should at least surround with "if" checking that "snapshot too old" is
enabled.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company