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
