On 10.05.2012 00:34, Tom Lane wrote:
After further study of the bgwriter hibernation patch (commit
6d90eaaa89a007e0d365f49d6436f35d2392cfeb), I think that my worries about
race conditions in the use of the bgwriter's latch are really the least
of its problems. BgBufferSync embodies a rather carefully tuned
feedback control loop, and I think these changes broke it. In the first
place, that feedback loop is built on the assumption that BgBufferSync
is executed at fixed intervals, which isn't true anymore; and in the
second place, the loop is not driven so much by the rate of buffers
being dirtied as it is by the rate of buffers being allocated. To be
concrete, if there is a constant but slow rate of buffer consumption,
the bgwriter will eventually lap the strategy scan and then stay there,
resulting in BgBufferSync returning true every time even though actually
the system is doing things. This results in bgwriter.c deciding it's in
hibernation mode, whereupon we have a scenario where backends will be
signaling it all the time. The way BgWriterNap is coded, that means
BgBufferSync is not executed at a fixed BgWriterDelay interval, but at
variable intervals from BgWriterDelay up to BGWRITER_HIBERNATE_MS, which
pretty much destroys the predictability of the feedback loop.
My proposal for fixing this is that
(1) BgBufferSync should return true (OK to hibernate) only if it's
lapped the strategy scan *and* recent_alloc is zero, meaning no new
buffers were allocated anywhere since last time.
(2) We should remove the bgwriter wakening calls from MarkBufferDirty
and SetBufferCommitInfoNeedsSave, and instead place one in buffer
allocation.
Hmm, that means that if you don't dirty any pages, bgwriter will wake up
even though it has no real work to do. It only wakes up to advance its
scan. I guess that's OK, if the system is busy doing read-only things
anyway, a few extra wakeups don't matter.
Waking bgwriter in buffer allocation makes sense also because the buffer
allocation stats will be updated more promptly that way. At the moment,
if the bgwriter hibernates, the bgwriter only updates the stats every 10
seconds.
(3) The bottom-of-loop logic in bgwriter should be along the lines of
rc = WaitLatch(..., BgWriterDelay);
if (rc == WL_TIMEOUT&& can_hibernate)
{
set global flag to tell backends to kick bgwriter
if they allocate a buffer;
WaitLatch(..., BGWRITER_HIBERNATE_MS);
clear global flag;
}
In comparison to the existing code, this method guarantees immediate
response to any signal (latch-setting event), and it ensures that
if we extend the normal sleep time for the bgwriter, the extra sleep
covers only an interval in which no new buffer allocations happened.
That provision seems to me to justify pretending that that interval
simply didn't exist for the purposes of the feedback control loop,
which allows us to not have to rethink how that loop works.
Comments?
Seems reasonable. Would you like me to write a patch, or are you already
on it?
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
