Horton

Thread

  1. WalSndWakeup() and synchronous_commit=off

    Andres Freund <andres@2ndquadrant.com> — 2012-05-10T19:51:20Z

    Hi all,

I noticed that when synchronous_commit=off were not waking up the wal sender 
latch in xact.c:RecordTransactionCommit which leads to ugly delays of approx 7 
seconds (1 + replication_timeout/10) with default settings.
Given that were flushing the wal to disk much sooner this appears to be a bad 
idea - especially as this may happen even under load if we ever reach the 
'coughtup' state.

I wonder why the WalSndWakeup isn't done like:

diff --git a/src/backend/access/transam/xlog.c 
b/src/backend/access/transam/xlog.c
index ecb71b6..7a3224b 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1906,6 +1906,10 @@ XLogWrite(XLogwrtRqst WriteRqst, bool flexible, bool 
xlog_switch)
            xlogctl->LogwrtRqst.Flush = LogwrtResult.Flush;
        SpinLockRelease(&xlogctl->info_lck);
    }
+
+   /* the walsender wasn't woken up in xact.c */
+   if(max_wal_senders > 1 && synchronous_commit == SYNCHRONOUS_COMMIT_OFF)
+       WalSndWakeup();
 }

Doing that for the synchronous_commit=off case can imo be considered a bugfix, 
but I wonder why we ever wake the senders somewhere else?
The only argument I can see for doing it at places like StartTransactionCommit 
is that thats the place after which the data will be visible on the client. I 
think thats a non-argument though because if wal is flushed to disk outside of 
a commit there normally is enough data to make it worthwile.

Doing the above results in a very noticeable reduction in lagginess and even a 
noticeable reduction in cpu-usage spikes on a busy replication test setup.

Greetings,

Andres

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

  2. Re: WalSndWakeup() and synchronous_commit=off

    Simon Riggs <simon@2ndquadrant.com> — 2012-05-11T09:36:01Z

    On 10 May 2012 20:51, Andres Freund <andres@2ndquadrant.com> wrote:

> I noticed that when synchronous_commit=off were not waking up the wal sender
> latch in xact.c:RecordTransactionCommit which leads to ugly delays of approx 7
> seconds (1 + replication_timeout/10) with default settings.
> Given that were flushing the wal to disk much sooner this appears to be a bad
> idea - especially as this may happen even under load if we ever reach the
> 'coughtup' state.

Sounds like a problem. I'll have a look.

> I wonder why the WalSndWakeup isn't done like:
>
> diff --git a/src/backend/access/transam/xlog.c
> b/src/backend/access/transam/xlog.c
> index ecb71b6..7a3224b 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -1906,6 +1906,10 @@ XLogWrite(XLogwrtRqst WriteRqst, bool flexible, bool
> xlog_switch)
>            xlogctl->LogwrtRqst.Flush = LogwrtResult.Flush;
>        SpinLockRelease(&xlogctl->info_lck);
>    }
> +
> +   /* the walsender wasn't woken up in xact.c */
> +   if(max_wal_senders > 1 && synchronous_commit == SYNCHRONOUS_COMMIT_OFF)
> +       WalSndWakeup();
>  }
>
> Doing that for the synchronous_commit=off case can imo be considered a bugfix,
> but I wonder why we ever wake the senders somewhere else?
> The only argument I can see for doing it at places like StartTransactionCommit
> is that thats the place after which the data will be visible on the client. I
> think thats a non-argument though because if wal is flushed to disk outside of
> a commit there normally is enough data to make it worthwile.
>
> Doing the above results in a very noticeable reduction in lagginess and even a
> noticeable reduction in cpu-usage spikes on a busy replication test setup.
>
> Greetings,
>
> Andres
>
> --
>  Andres Freund                     http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers



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

  3. Re: WalSndWakeup() and synchronous_commit=off

    Fujii Masao <masao.fujii@gmail.com> — 2012-05-11T17:09:52Z

    On Fri, May 11, 2012 at 4:51 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> diff --git a/src/backend/access/transam/xlog.c
> b/src/backend/access/transam/xlog.c
> index ecb71b6..7a3224b 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -1906,6 +1906,10 @@ XLogWrite(XLogwrtRqst WriteRqst, bool flexible, bool
> xlog_switch)
>            xlogctl->LogwrtRqst.Flush = LogwrtResult.Flush;
>        SpinLockRelease(&xlogctl->info_lck);
>    }
> +
> +   /* the walsender wasn't woken up in xact.c */
> +   if(max_wal_senders > 1 && synchronous_commit == SYNCHRONOUS_COMMIT_OFF)
> +       WalSndWakeup();
>  }

Calling WalSndWakeup() while WALWriteLock is being held might cause another
performance degradation. No?

Regards,

-- 
Fujii Masao

  4. Re: WalSndWakeup() and synchronous_commit=off

    Robert Haas <robertmhaas@gmail.com> — 2012-05-11T17:20:26Z

    On Fri, May 11, 2012 at 1:09 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> Calling WalSndWakeup() while WALWriteLock is being held might cause another
> performance degradation. No?

That definitely doesn't seem ideal - a lot of things can pile up
behind WALWriteLock.  I'm not sure how big a problem it would be in
practice, but we generally make a practice of avoiding sending signals
while holding LWLocks whenever possible...

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

  5. Re: WalSndWakeup() and synchronous_commit=off

    Andres Freund <andres@2ndquadrant.com> — 2012-05-11T18:01:17Z

    On Friday, May 11, 2012 07:20:26 PM Robert Haas wrote:
> On Fri, May 11, 2012 at 1:09 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> > Calling WalSndWakeup() while WALWriteLock is being held might cause
> > another performance degradation. No?
> 
> That definitely doesn't seem ideal - a lot of things can pile up
> behind WALWriteLock.  I'm not sure how big a problem it would be in
> practice, but we generally make a practice of avoiding sending signals
> while holding LWLocks whenever possible...
In my measurements on moderately powerful hardware I couldn't see any 
degradation on the primary - in fact the contrary, but the improvements were 
around 0.4% and I only tested 10min so its not exactly hard evidence. But I 
aggree its not ideal.
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.

Better Ideas?

Andres

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

  6. Re: WalSndWakeup() and synchronous_commit=off

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-05-11T18:36:24Z

    Robert Haas <robertmhaas@gmail.com> writes:
> That definitely doesn't seem ideal - a lot of things can pile up
> behind WALWriteLock.  I'm not sure how big a problem it would be in
> practice, but we generally make a practice of avoiding sending signals
> while holding LWLocks whenever possible...

There's a good reason for that, which is that the scheduler might well
decide to go run the wakened process instead of you.  Admittedly this
tends to not be a problem on machines with $bignum CPUs, but on
single-CPU machines I've seen it happen a lot.

Refactoring so that the signal is sent only after lock release seems
like a good idea to me.

			regards, tom lane

  7. Re: WalSndWakeup() and synchronous_commit=off

    Andres Freund <andres@2ndquadrant.com> — 2012-05-11T18:40:40Z

    On Friday, May 11, 2012 08:36:24 PM Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > That definitely doesn't seem ideal - a lot of things can pile up
> > behind WALWriteLock.  I'm not sure how big a problem it would be in
> > practice, but we generally make a practice of avoiding sending signals
> > while holding LWLocks whenever possible...
> 
> There's a good reason for that, which is that the scheduler might well
> decide to go run the wakened process instead of you.  Admittedly this
> tends to not be a problem on machines with $bignum CPUs, but on
> single-CPU machines I've seen it happen a lot.
> 
> Refactoring so that the signal is sent only after lock release seems
> like a good idea to me.
Will send a patch lateron, duplication seems to be manageable.

Andres

  8. Re: WalSndWakeup() and synchronous_commit=off

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-05-11T18:45:23Z

    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?

			regards, tom lane

  9. Re: WalSndWakeup() and synchronous_commit=off

    Simon Riggs <simon@2ndquadrant.com> — 2012-05-11T20:45:42Z

    On 11 May 2012 19:45, Tom Lane <tgl@sss.pgh.pa.us> 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?

I think because nobody thought of that.

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

  10. Re: WalSndWakeup() and synchronous_commit=off

    Andres Freund <andres@2ndquadrant.com> — 2012-05-14T09:32:35Z

    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.

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

  11. Re: WalSndWakeup() and synchronous_commit=off

    Fujii Masao <masao.fujii@gmail.com> — 2012-05-14T17:55:32Z

    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.

Regards,

-- 
Fujii Masao

  12. Re: WalSndWakeup() and synchronous_commit=off

    Andres Freund <andres@2ndquadrant.com> — 2012-05-15T15:30:27Z

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

Andres

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

  13. Re: WalSndWakeup() and synchronous_commit=off

    Andres Freund <andres@2ndquadrant.com> — 2012-05-28T14:35:32Z

    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

  14. Re: WalSndWakeup() and synchronous_commit=off

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-05-28T17:11:53Z

    Andres Freund <andres@2ndquadrant.com> writes:
> 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?

My concern was basically that it might throw an error.  Looking at the
current implementation of SetLatch, it seems that's not possible, but
I wonder whether we want to lock ourselves into that assumption.

Still, if the alternatives are worse, maybe that's the best answer.
If we do that, though, let's add comments to WalSndWakeup and SetLatch
mentioning that they mustn't throw error.

			regards, tom lane

  15. Re: WalSndWakeup() and synchronous_commit=off

    Andres Freund <andres@2ndquadrant.com> — 2012-05-29T18:42:43Z

    Hi,

On Monday, May 28, 2012 07:11:53 PM Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > 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?
> 
> My concern was basically that it might throw an error.  Looking at the
> current implementation of SetLatch, it seems that's not possible, but
> I wonder whether we want to lock ourselves into that assumption.
The assumption is already made at several other places I think. 
XLogSetAsyncXactLSN does a SetLatch and is called from critical sections; 
several signal handlers call it without any attention to the context.

Requiring it to be called outside would make its usage considerably less 
convenient and I don't really see what could change that would require to 
throw non-panic errors.

> Still, if the alternatives are worse, maybe that's the best answer.
> If we do that, though, let's add comments to WalSndWakeup and SetLatch
> mentioning that they mustn't throw error.
Patch attached.

Greetings,

Andres


PS: Sorry for dropping the CC list before...

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

  16. Re: WalSndWakeup() and synchronous_commit=off

    Andres Freund <andres@2ndquadrant.com> — 2012-05-30T12:46:19Z

    On Tuesday, May 29, 2012 08:42:43 PM Andres Freund wrote:
> Patch attached.
Imo this patch should be backported to 9.1, 9.0 doesn't use latches and does 
not do explicit wakeup of the sender so its not applicable there.

I can prepare a patch for 9.1 if people agree, there has been some amount of 
change that won't make it apply cleanly.

Greetings,

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

  17. Re: WalSndWakeup() and synchronous_commit=off

    Fujii Masao <masao.fujii@gmail.com> — 2012-05-30T23:33:33Z

    On Wed, May 30, 2012 at 9:46 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On Tuesday, May 29, 2012 08:42:43 PM Andres Freund wrote:
>> Patch attached.
> Imo this patch should be backported to 9.1, 9.0 doesn't use latches and does
> not do explicit wakeup of the sender so its not applicable there.
>
> I can prepare a patch for 9.1 if people agree, there has been some amount of
> change that won't make it apply cleanly.

The patch wakes up walsender more frequently than now. Which leads
walsender to send smaller WAL data packet more frequently, and furthermore
which leads walreceiver to issue fsync more frequently. So I'm afraid that
the patch makes the disk more busy and slows down the read-only query
in the standby. I'm also afraid that frequent fsync calls degrade the
performance
of sync replication. So it's better to do benchmark to address the concerns.

Regards,

-- 
Fujii Masao

  18. Re: WalSndWakeup() and synchronous_commit=off

    Andres Freund <andres@2ndquadrant.com> — 2012-05-30T23:43:07Z

    On Thursday, May 31, 2012 01:33:33 AM Fujii Masao wrote:
> On Wed, May 30, 2012 at 9:46 PM, Andres Freund <andres@2ndquadrant.com> 
wrote:
> > On Tuesday, May 29, 2012 08:42:43 PM Andres Freund wrote:
> >> Patch attached.
> > 
> > Imo this patch should be backported to 9.1, 9.0 doesn't use latches and
> > does not do explicit wakeup of the sender so its not applicable there.
> > 
> > I can prepare a patch for 9.1 if people agree, there has been some amount
> > of change that won't make it apply cleanly.
> 
> The patch wakes up walsender more frequently than now. Which leads
> walsender to send smaller WAL data packet more frequently, and furthermore
> which leads walreceiver to issue fsync more frequently. So I'm afraid that
> the patch makes the disk more busy and slows down the read-only query
> in the standby. I'm also afraid that frequent fsync calls degrade the
> performance
> of sync replication. So it's better to do benchmark to address the
> concerns.
I couldn't measure any significant difference in #fsyncs or replication speed 
in a busy pgbench workload. If anything there were less, but the difference 
was small. Both with synchronous_commit=on/off. I did *not* test sync repl.

Why would you expect a change? Walsender is only signalled if XLogWrite 
actually flushed data to disk. Thats the point of the exercise. Thats normally 
only done if the wal buffers are full or a commit record (+some other stuff) 
requires the xlog to be flushed up to some point.

Greetings,

Andres

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

  19. Re: WalSndWakeup() and synchronous_commit=off

    Andres Freund <andres@2ndquadrant.com> — 2012-06-06T19:11:02Z

    On Tuesday, May 29, 2012 08:42:43 PM Andres Freund wrote:
> Hi,
> 
> On Monday, May 28, 2012 07:11:53 PM Tom Lane wrote:
> > Andres Freund <andres@2ndquadrant.com> writes:
> > > 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?
> > 
> > My concern was basically that it might throw an error.  Looking at the
> > current implementation of SetLatch, it seems that's not possible, but
> > I wonder whether we want to lock ourselves into that assumption.
> 
> The assumption is already made at several other places I think.
> XLogSetAsyncXactLSN does a SetLatch and is called from critical sections;
> several signal handlers call it without any attention to the context.
> 
> Requiring it to be called outside would make its usage considerably less
> convenient and I don't really see what could change that would require to
> throw non-panic errors.
> 
> > Still, if the alternatives are worse, maybe that's the best answer.
> > If we do that, though, let's add comments to WalSndWakeup and SetLatch
> > mentioning that they mustn't throw error.
> 
> Patch attached.
I would like to invite some more review (+commit...) here ;). Imo this is an 
annoying bug which should be fixed before next point release or beta/rc comes 
out...

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

  20. Re: WalSndWakeup() and synchronous_commit=off

    Simon Riggs <simon@2ndquadrant.com> — 2012-06-07T18:41:23Z

    On 6 June 2012 20:11, Andres Freund <andres@2ndquadrant.com> wrote:
> On Tuesday, May 29, 2012 08:42:43 PM Andres Freund wrote:
>> Hi,
>>
>> On Monday, May 28, 2012 07:11:53 PM Tom Lane wrote:
>> > Andres Freund <andres@2ndquadrant.com> writes:
>> > > 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?
>> >
>> > My concern was basically that it might throw an error.  Looking at the
>> > current implementation of SetLatch, it seems that's not possible, but
>> > I wonder whether we want to lock ourselves into that assumption.
>>
>> The assumption is already made at several other places I think.
>> XLogSetAsyncXactLSN does a SetLatch and is called from critical sections;
>> several signal handlers call it without any attention to the context.
>>
>> Requiring it to be called outside would make its usage considerably less
>> convenient and I don't really see what could change that would require to
>> throw non-panic errors.
>>
>> > Still, if the alternatives are worse, maybe that's the best answer.
>> > If we do that, though, let's add comments to WalSndWakeup and SetLatch
>> > mentioning that they mustn't throw error.
>>
>> Patch attached.
> I would like to invite some more review (+commit...) here ;). Imo this is an
> annoying bug which should be fixed before next point release or beta/rc comes
> out...

Moved the wakeup to a logical place outside a critical section.

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

  21. Re: WalSndWakeup() and synchronous_commit=off

    Andres Freund <andres@2ndquadrant.com> — 2012-06-07T20:08:10Z

    On Thursday, June 07, 2012 08:41:23 PM Simon Riggs wrote:
> On 6 June 2012 20:11, Andres Freund <andres@2ndquadrant.com> wrote:
> > On Tuesday, May 29, 2012 08:42:43 PM Andres Freund wrote:
> >> Hi,
> >> 
> >> On Monday, May 28, 2012 07:11:53 PM Tom Lane wrote:
> >> > Andres Freund <andres@2ndquadrant.com> writes:
> >> > > 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?
> >> > 
> >> > My concern was basically that it might throw an error.  Looking at the
> >> > current implementation of SetLatch, it seems that's not possible, but
> >> > I wonder whether we want to lock ourselves into that assumption.
> >> 
> >> The assumption is already made at several other places I think.
> >> XLogSetAsyncXactLSN does a SetLatch and is called from critical
> >> sections; several signal handlers call it without any attention to the
> >> context.
> >> 
> >> Requiring it to be called outside would make its usage considerably less
> >> convenient and I don't really see what could change that would require
> >> to throw non-panic errors.
> >> 
> >> > Still, if the alternatives are worse, maybe that's the best answer.
> >> > If we do that, though, let's add comments to WalSndWakeup and SetLatch
> >> > mentioning that they mustn't throw error.
> >> 
> >> Patch attached.
> > 
> > I would like to invite some more review (+commit...) here ;). Imo this is
> > an annoying bug which should be fixed before next point release or
> > beta/rc comes out...
> 
> Moved the wakeup to a logical place outside a critical section.
Hm. I don't really like the way you implemented that. While it reduces the 
likelihood quite a bit it will still miss wakeups if an XLogInsert pushes out 
the data because of missing space or if any place does an XLogFlush(lsn).
The knowledge is really only available in XLogWrite...

Andres

  22. Re: WalSndWakeup() and synchronous_commit=off

    Simon Riggs <simon@2ndquadrant.com> — 2012-06-07T23:42:22Z

    On 7 June 2012 21:08, Andres Freund <andres@2ndquadrant.com> wrote:

>> Moved the wakeup to a logical place outside a critical section.

> Hm. I don't really like the way you implemented that. While it reduces the
> likelihood quite a bit it will still miss wakeups if an XLogInsert pushes out
> the data because of missing space or if any place does an XLogFlush(lsn).
> The knowledge is really only available in XLogWrite...

Right, but the placement inside the critical section was objected to.

This way, any caller of XLogFlush() will be swept up at least once per
wal_writer_delay, so missing a few calls doesn't mean we have spikes
in replication delay.

Doing it more frequently was also an objection from Fujii, to which we
must listen.

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

  23. Re: WalSndWakeup() and synchronous_commit=off

    Andres Freund <andres@2ndquadrant.com> — 2012-06-08T08:10:29Z

    Hi,

On Friday, June 08, 2012 01:42:22 AM Simon Riggs wrote:
> On 7 June 2012 21:08, Andres Freund <andres@2ndquadrant.com> wrote:
> >> Moved the wakeup to a logical place outside a critical section.
> > 
> > Hm. I don't really like the way you implemented that. While it reduces
> > the likelihood quite a bit it will still miss wakeups if an XLogInsert
> > pushes out the data because of missing space or if any place does an
> > XLogFlush(lsn). The knowledge is really only available in XLogWrite...
> 
> Right, but the placement inside the critical section was objected to.
And that objection was later somewhat eased by Tom. There currently still are 
several WalSndWakeup calls in critical sections and several other uses of 
latches in critial sections and several in signal handlers (which may be 
during a critical section). Thats why I added comments to SetLatch documenting 
that they need to be signal/concurrency safe. (Which they are atm)

> This way, any caller of XLogFlush() will be swept up at least once per
> wal_writer_delay, so missing a few calls doesn't mean we have spikes
> in replication delay.
Unfortunately it does. At least I measure it here ;) (obviously less than the 
prev. 7 seconds). Its not that surprising though: Especially during high or 
even more so during bursty wal activity a backend might decide to write out 
wal itself. In that case the background writer doesn't necessarily have 
anything to write out so it won't wake the walsender.
Under high load the number of wakeups is twice or thrice as high *before* my 
patch than afterwards (with synchronous_commit=on obviously). As you most 
definitely know (group commit work et al) in a concurrent situation many of 
the wakeups from the current location (RecordTransactionCommit) are useless 
because the xlog was already flushed by another backend/background writer 
before we get to do it.

> Doing it more frequently was also an objection from Fujii, to which we
> must listen.
I had hoped that I argued successfully against that, but obviously not ;)

Greetings,

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