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).
> 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.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
