Re: [PATCH 01/16] Overhaul walsender wakeup handling

On Fri, Jun 22, 2012 at 10:19 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On Friday, June 22, 2012 04:09:59 PM Robert Haas wrote:
>> >> I am not convinced that it's a good idea to wake up every walsender
>> >> every time we do XLogInsert().  XLogInsert() is a super-hot code path,
>> >> and adding more overhead there doesn't seem warranted.  We need to
>> >> replicate commit, commit prepared, etc. quickly, by why do we need to
>> >> worry about a short delay in replicating heap_insert/update/delete,
>> >> for example?  They don't really matter until the commit arrives.  7
>> >> seconds might be a bit long, but that could be fixed by decreasing the
>> >> polling interval for walsender to, say, a second.
>> >
>> > Its not woken up every XLogInsert call. Its only woken up if there was an
>> > actual disk write + fsync in there. Thats exactly the point of the patch.
>> Sure, but it's still adding cycles to XLogInsert.  I'm not sure that
>> XLogBackgroundFlush() is the right place to be doing this, but at
>> least it's in the background rather than the foreground.
> It adds one if() if nothing was fsynced. If something was written and fsynced
> inside XLogInsert some kill() calls are surely not the problem.
>
>> > The wakeup rate is actually lower for synchronous_commit=on than before
>> > because then it unconditionally did a wakeup for every commit (and
>> > similar) and now only does that if something has been written + fsynced.
>> I'm a bit confused by this, because surely if there's been a commit,
>> then WAL has been written and fsync'd, but the reverse is not true.
> As soon as you have significant concurrency by the time the XLogFlush in
> RecordTransactionCommit() is reached another backend or the wal writer may
> have already fsynced the wal up to the requested point. In that case no wakeup
> will performed by the comitting backend at all. 9.2 improved the likelihood of
> that as you know.

Hmm, well, I guess. I'm still not sure I really understand what
benefit we're getting out of this. If we lose a few WAL records for
an uncommitted transaction, who cares? That transaction is gone
anyway.

As an implementation detail, I suggest rewriting WalSndWakeupRequest
and WalSndWakeupProcess as macros. The old code does an in-line test
for max_wal_senders > 0, which suggests that somebody thought the
function call overhead might be enough to matter here. Perhaps they
were wrong, but it shouldn't hurt anything to keep it that way.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company