Re: Latch for the WAL writer - further reducing idle wake-ups.

Peter Geoghegan <peter(at)2ndquadrant(dot)com> writes:
> Attached patch latches up the WAL Writer, reducing wake-ups and thus
> saving electricity in a way that is more-or-less analogous to my work
> on the BGWriter:
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=6d90eaaa89a007e0d365f49d6436f35d2392cfeb
> I am hoping this gets into 9.2 . I am concious of the fact that this
> is quite late, but it the patch addresses an open item, the concluding
> part of a much wider feature.

It is getting a bit late to be considering such changes for 9.2, but
I'm willing to review and commit this if there's not anybody who feels
strongly that it's too late. Personally I think it's in the nature of
cleanup and so fair game as long as we haven't formally started beta.
However I will confess to some bias about wanting to get the server's
idle wake-up rate down, because Fedora people have been bugging me
about that for a long time now. So I'm probably not the best person to
objectively evaluate whether we should hold this for 9.3. Comments?

Schedule questions aside, I'm disturbed by this bit:

> My choice of XLogInsert() as an additional site at which to call
> SetLatch() was one that wasn't taken easily, and frankly I'm not
> entirely confident that I couldn't have been just as effective while
> placing the SetLatch() call in a less hot, perhaps higher-level
> codepath.

Adding any contention at all to XLogInsert doesn't seem like a smart
idea, even if you failed to measure any problem in the specific tests
you made. I wonder whether we could not improve matters by adding
an additional bool "wal_writer_needs_wakening" in the state that's
considered to be protected by WALInsertLock. XLogInsert would check
this while still holding the lock, and only consider that it needs to do
a SetLatch if the flag was set, whereupon it would clear it before
releasing the lock. In the normal case this would add one uncontended
fetch followed by boolean-test-and-jump to the work done while holding
the lock, which should be pretty negligible. Then, the WAL writer would
need to take WALInsertLock to set that flag, but presumably it should
only be doing that when there is no contention for the lock. (In fact,
we could have it do a ConditionalLockAcquire on WALInsertLock for the
purpose, and consider that failure means it shouldn't go to sleep after
all.)

Now this might sound pretty much equivalent to testing the latch's
is_set flag; perhaps it is and I'm worrying over nothing. But I'm
thinking that the wal_writer_needs_wakening flag would be in a cache
line that an acquirer of WALInsertLock would have to get ownership of
anyway, if it is adjacent to variables that XLogInsert has to manipulate
anyway. On the other hand, the WAL writer's process latch would be in
some other cache line that would also need to get passed around a lot,
if it's touched during every XLogInsert.

regards, tom lane