On Mon, Apr 11, 2016 at 4:27 PM, Alvaro Herrera <alvhe...@2ndquadrant.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
