Re: [COMMITTERS] pgsql: Make walsender more responsive.

Make walsender more responsive.

Per testing by Andres Freund, this improves replication performance
and reduces replication latency and latency jitter. I was a bit
concerned about moving more work into XLogInsert, but testing seems
to show that it's not a problem in practice.

Along the way, improve comments for WaitLatchOrSocket.

Andres Freund. Review and stylistic cleanup by me.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/f83b59997d29f06c3d67e7eb9a1f2c9cd017d665

Modified Files
--------------
src/backend/access/transam/twophase.c | 21 ---------------------
src/backend/access/transam/xact.c | 7 -------
src/backend/access/transam/xlog.c | 25 ++++++++++++++++++-------
src/backend/port/unix_latch.c | 3 +++
src/backend/port/win32_latch.c | 4 ++++
src/backend/replication/walsender.c | 11 ++++++++++-
src/include/replication/walsender.h | 24 ++++++++++++++++++++++++
7 files changed, 59 insertions(+), 36 deletions(-)

Robert Haas <rhaas(at)postgresql(dot)org> writes:
> Make walsender more responsive. ...
> Andres Freund. Review and stylistic cleanup by me.

The comments could have used a bit more copy-editing.

(I got a good laugh out of the idea of "contented locks".)

regards, tom lane

On Mon, Jul 2, 2012 at 10:34 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <rhaas(at)postgresql(dot)org> writes:
>> Make walsender more responsive. ...
>> Andres Freund. Review and stylistic cleanup by me.
>
> The comments could have used a bit more copy-editing.
>
> (I got a good laugh out of the idea of "contented locks".)

Uh... what? I thought the meaning of that was perfectly clear.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Jul 2, 2012 at 10:34 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> (I got a good laugh out of the idea of "contented locks".)

> Uh... what? I thought the meaning of that was perfectly clear.

My dictionary defines "contented" as "happy and at ease". I think
the word intended here is "contended":

+ /* wakeup the WalSnd now that we outside contented locks */

not to mention that this sentence no verb. I don't hold such things
against patch submitters whose first language isn't English, but
committers whose first language *is* English ought to fix them.

regards, tom lane

On Mon, Jul 2, 2012 at 11:06 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Jul 2, 2012 at 10:34 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> (I got a good laugh out of the idea of "contented locks".)
>
>> Uh... what? I thought the meaning of that was perfectly clear.
>
> My dictionary defines "contented" as "happy and at ease". I think
> the word intended here is "contended":

Oh, nuts. My eyes skimmed right over that one after you pointed it out.

> + /* wakeup the WalSnd now that we outside contented locks */
>
> not to mention that this sentence no verb. I don't hold such things
> against patch submitters whose first language isn't English, but
> committers whose first language *is* English ought to fix them.

OK.

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

On Mon, Jul 02, 2012 at 10:34:34AM -0400, Tom Lane wrote:
> Robert Haas <rhaas(at)postgresql(dot)org> writes:
> > Make walsender more responsive. ... Andres Freund. Review and
> > stylistic cleanup by me.
>
> The comments could have used a bit more copy-editing.
>
> (I got a good laugh out of the idea of "contented locks".)

I say we leave it in there ;)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Mon, Jul 2, 2012 at 10:49 PM, Robert Haas <rhaas(at)postgresql(dot)org> wrote:
> Make walsender more responsive.
>
> Per testing by Andres Freund, this improves replication performance
> and reduces replication latency and latency jitter. I was a bit
> concerned about moving more work into XLogInsert, but testing seems
> to show that it's not a problem in practice.
>
> Along the way, improve comments for WaitLatchOrSocket.

This commit makes the synchronous replication slow down very much
when wal_sync_method is set to open_sync or open_datasync. I think
the attached patch needs to be applied.

+#define WalSndWakeupProcessRequests() \
+ do \
+ { \
+ if (wake_wal_senders) \
+ { \
+ wake_wal_senders = false; \
+ if (max_wal_senders > 0) \
+ WalSndWakeup(); \
+ } \
+ } while (0)

I'm not sure it's really worth doing, but isn't it good idea to test
max_wal_sender > 0 first to eliminate any CPU cycle in non replication case?

Regards,

--
Fujii Masao

On Monday, July 02, 2012 07:19:19 PM Fujii Masao wrote:
> On Mon, Jul 2, 2012 at 10:49 PM, Robert Haas <rhaas(at)postgresql(dot)org> wrote:
> > Make walsender more responsive.
> >
> > Per testing by Andres Freund, this improves replication performance
> > and reduces replication latency and latency jitter. I was a bit
> > concerned about moving more work into XLogInsert, but testing seems
> > to show that it's not a problem in practice.
> >
> > Along the way, improve comments for WaitLatchOrSocket.
>
> This commit makes the synchronous replication slow down very much
> when wal_sync_method is set to open_sync or open_datasync. I think
> the attached patch needs to be applied.
Hm. Yes, definitely. No idea why I placed the call there, sorry.

Thats how synchronous_write=off behaved generally till the recent (simple) fix
btw ;)

> +#define WalSndWakeupProcessRequests() \
> + do \
> + { \
> + if (wake_wal_senders) \
> + { \
> + wake_wal_senders = false; \
> + if (max_wal_senders > 0) \
> + WalSndWakeup(); \
> + } \
> + } while (0)
>
> I'm not sure it's really worth doing, but isn't it good idea to test
> max_wal_sender > 0 first to eliminate any CPU cycle in non replication
> case?
I think the difference is ignorable. wake_wal_senders probably has better
cache locality but is set to true more often, but not that often...

Thanks,

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

On Mon, Jul 2, 2012 at 1:53 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> This commit makes the synchronous replication slow down very much
>> when wal_sync_method is set to open_sync or open_datasync. I think
>> the attached patch needs to be applied.
> Hm. Yes, definitely. No idea why I placed the call there, sorry.

Committed.

>> +#define WalSndWakeupProcessRequests() \
>> + do \
>> + { \
>> + if (wake_wal_senders) \
>> + { \
>> + wake_wal_senders = false; \
>> + if (max_wal_senders > 0) \
>> + WalSndWakeup(); \
>> + } \
>> + } while (0)
>>
>> I'm not sure it's really worth doing, but isn't it good idea to test
>> max_wal_sender > 0 first to eliminate any CPU cycle in non replication
>> case?
> I think the difference is ignorable. wake_wal_senders probably has better
> cache locality but is set to true more often, but not that often...

I was wondering if we shouldn't do this as:

if (max_wal_senders > 0 && wake_wal_senders)
WalSndWakeup();

....and then put wake_wal_senders = false into WalSndWakeup().

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