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@postgresql.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@sss.pgh.pa.us> wrote:
Robert Haas <rhaas@postgresql.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@gmail.com> writes:
On Mon, Jul 2, 2012 at 10:34 AM, Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Mon, Jul 2, 2012 at 10:34 AM, Tom Lane <tgl@sss.pgh.pa.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@postgresql.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@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.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@postgresql.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
Attachments:
bugfix_v1.patchapplication/octet-stream; name=bugfix_v1.patchDownload
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
***************
*** 1867,1876 **** XLogWrite(XLogwrtRqst WriteRqst, bool flexible, bool xlog_switch)
}
issue_xlog_fsync(openLogFile, openLogSegNo);
-
- /* signal that we need to wakeup walsenders later */
- WalSndWakeupRequest();
}
LogwrtResult.Flush = LogwrtResult.Write;
}
--- 1867,1877 ----
}
issue_xlog_fsync(openLogFile, openLogSegNo);
}
+
+ /* signal that we need to wakeup walsenders later */
+ WalSndWakeupRequest();
+
LogwrtResult.Flush = LogwrtResult.Write;
}
On Monday, July 02, 2012 07:19:19 PM Fujii Masao wrote:
On Mon, Jul 2, 2012 at 10:49 PM, Robert Haas <rhaas@postgresql.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@2ndquadrant.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