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

On Friday, June 22, 2012 04:34:33 PM Robert Haas wrote:
> On Fri, Jun 22, 2012 at 10:19 AM, Andres Freund <andres@2ndquadrant.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.
Well, before the simple fix Simon applied after my initial complaint you 
didn't get wakeups *at all* in the synchronous_commit=off case.

Now, with the additional changes, the walsender is woken exactly when data is 
available to send and not always when a commit happens. I played around with 
various scenarios and it always was a win. One reason is that the walreceiver 
often is a bottleneck because it fsyncs the received data immediately so a 
less blocky transfer pattern is reducing that problem a bit.

> 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.
True.

Greetings,

Andres
-- 
 Andres Freund	                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services