Re: WalSndWakeup() and synchronous_commit=off

On Tuesday, May 15, 2012 05:30:27 PM Andres Freund wrote:
> On Monday, May 14, 2012 07:55:32 PM Fujii Masao wrote:
> > On Mon, May 14, 2012 at 6:32 PM, Andres Freund <andres@2ndquadrant.com>
> 
> wrote:
> > > On Friday, May 11, 2012 08:45:23 PM Tom Lane wrote:
> > >> Andres Freund <andres@2ndquadrant.com> writes:
> > >> > Its the only place though which knows whether its actually sensible
> > >> > to wakeup the walsender. We could make it return whether it wrote
> > >> > anything and do the wakeup at the callers. I count 4 different
> > >> > callsites which would be an annoying duplication but I don't really
> > >> > see anything better right now.
> > >> 
> > >> Another point here is that XLogWrite is not only normally called with
> > >> the lock held, but inside a critical section.  I see no reason to take
> > >> the risk of doing signal sending inside critical sections.
> > >> 
> > >> BTW, a depressingly large fraction of the existing calls to
> > >> WalSndWakeup are also inside critical sections, generally for no good
> > >> reason that I can see.  For example, in EndPrepare(), why was the
> > >> call placed where it is and not down beside SyncRepWaitForLSN?
> > > 
> > > Hm. While I see no real problem moving it out of the lock I don't
> > > really see a way to cleanly outside critical sections everywhere. The
> > > impact of doing so seems to be rather big to me. The only externally
> > > visible place where it actually is known whether we write out data and
> > > thus do the wakeup is XLogInsert, XLogFlush and XLogBackgroundFlush.
> > > The first two of those are routinely called inside a critical section.
> > 
> > So what about moving the existing calls of WalSndWakeup() out of a
> > critical section and adding new call of WalSndWakeup() into
> > XLogBackgroundFlush()? Then all WalSndWakeup()s are called outside a
> > critical section and after releasing WALWriteLock. I attached the patch.
> 
> Imo its simply not sensible to call WalSndWakeup at *any* of the current
> locations. They simply don't have the necessary information. They will
> wakeup too often (because with concurrency commits often won't require
> additional wal writes) and too seldom (because a flush caused by
> XLogInsert wont cause a wakeup).
Does anybody have a better idea than to either call WalSndWakeup() at 
essentially the wrong places or calling it inside a critical section?

Tom, what danger do you see from calling it in a critical section?

Greetings,

Andres