Thank you very much for your review.
I generally agree with your suggestions, so just applied them.
You can find the new patch in the attached file.
Best
Majid
On Tue, 6 Feb 2024 at 09:26, Michael Paquier <mich...@paquier.xyz> wrote:
> On Fri, Jan 19, 2024 at 11:04:50PM +0330, Majid Garoosi wrote:
> > However, this value does not need to be larger than wal_segment_size,
> > thus its checker function returns false if a larger value is set for
> > this.
> >
> > This is my first patch. So, I hope I haven't done something wrong. :'D
>
> You've done nothing wrong. Thanks for the patch!
>
> + if (*newval > wal_segment_size)
> + return false;
> + return true;
>
> I was not sure first that we need a dynamic check, but I could get why
> somebody may want to make it higher than 1MB these days.
>
> The patch is missing a couple of things:
> - Documentation in doc/src/sgml/config.sgml, that has a section for
> "Sending Servers".
> - It is not listed in postgresql.conf.sample. I would suggest to put
> it in REPLICATION -> Sending Servers.
> The default value of 128kB should be mentioned in both cases.
>
> - * We don't have a good idea of what a good value would be; there's some
> - * overhead per message in both walsender and walreceiver, but on the
> other
> - * hand sending large batches makes walsender less responsive to signals
> - * because signals are checked only between messages. 128kB (with
> - * default 8k blocks) seems like a reasonable guess for now.
> [...]
> + gettext_noop("Walsender procedure consists of a loop, reading
> wal_sender_max_send_size "
> + "bytes of WALs from disk and sending them to the receiver.
> Sending large "
> + "batches makes walsender less responsive to signals."),
>
> This is removing some information about why it may be a bad idea to
> use a too low value (message overhead) and why it may be a bad idea to
> use a too large value (responsiveness). I would suggest to remove the
> second gettext_noop() in guc_tables.c and move all this information to
> config.sgml with the description of the new GUC. Perhaps just put it
> after wal_sender_timeout in the sample file and the docs?
>
> Three comments in walsender.c still mention MAX_SEND_SIZE. These
> should be switched to mention the GUC instead.
>
> I am switching the patch as waiting on author for now.
> --
> Michael
>
From fdd833b8b5cfdc1645f8288b27e9a92fc46f7951 Mon Sep 17 00:00:00 2001
From: Majid Garoosi <amoomaji...@gmail.com>
Date: Fri, 19 Jan 2024 22:48:23 +0330
Subject: [PATCH v2] Add documentation for wal_sender_max_send_size
Detailed changes:
- Add documentation to postgresql's config docs
- Remove code comments mentioning the setting's old constant name
- Add the parameter to postgresql.conf.sample
---
doc/src/sgml/config.sgml | 26 ++++++++++++++++++
src/backend/replication/walsender.c | 27 +++++++------------
src/backend/utils/init/postinit.c | 11 ++++++++
src/backend/utils/misc/guc_tables.c | 11 ++++++++
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/include/replication/walsender.h | 1 +
src/include/utils/guc_hooks.h | 1 +
7 files changed, 61 insertions(+), 17 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 61038472c5..86b9488cd1 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4405,6 +4405,32 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows
</listitem>
</varlistentry>
+ <varlistentry id="guc-wal-sender-max-send-size" xreflabel="wal_sender_max_send_size">
+ <term><varname>wal_sender_max_send_size</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>wal_sender_max_send_size</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Maximum size of chunks that wal sender reads from disk and sends to the
+ standby servers in each iteration. Its default value is <literal>16</literal>
+ times <literal>XLOG_BLCKSZ</literal>, i.e. it is <literal>128kB</literal> if
+ the project is built with the default <literal>XLOG_BLCKSZ</literal>
+ configuration parameter. This parameter can be changed without restarting the
+ server.
+ </para>
+ <para>
+ The minimum allowed value is <literal>XLOG_BLCKSZ</literal> bytes, typically
+ <literal>8kB</literal>. Its maximum allowed value is
+ <varname>wal_segment_size</varname>, typically <literal>16MB</literal>.
+ While a large value for this parameter reduces the responsiveness of WAL
+ sender processes to the signals, setting small values increases the overhead
+ of the messages both in WAL senders and WAL receivers.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-track-commit-timestamp" xreflabel="track_commit_timestamp">
<term><varname>track_commit_timestamp</varname> (<type>boolean</type>)
<indexterm>
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 087031e9dc..c429facded 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -96,17 +96,6 @@
#include "utils/timeout.h"
#include "utils/timestamp.h"
-/*
- * Maximum data payload in a WAL data message. Must be >= XLOG_BLCKSZ.
- *
- * We don't have a good idea of what a good value would be; there's some
- * overhead per message in both walsender and walreceiver, but on the other
- * hand sending large batches makes walsender less responsive to signals
- * because signals are checked only between messages. 128kB (with
- * default 8k blocks) seems like a reasonable guess for now.
- */
-#define MAX_SEND_SIZE (XLOG_BLCKSZ * 16)
-
/* Array of WalSnds in shared memory */
WalSndCtlData *WalSndCtl = NULL;
@@ -124,6 +113,9 @@ int max_wal_senders = 10; /* the maximum number of concurrent
* walsenders */
int wal_sender_timeout = 60 * 1000; /* maximum time to send one WAL
* data message */
+int wal_sender_max_send_size = XLOG_BLCKSZ * 16; /* Maximum data
+ * payload in a WAL
+ * data message */
bool log_replication_commands = false;
/*
@@ -2894,8 +2886,8 @@ WalSndSegmentOpen(XLogReaderState *state, XLogSegNo nextSegNo,
/*
* Send out the WAL in its normal physical/stored form.
*
- * Read up to MAX_SEND_SIZE bytes of WAL that's been flushed to disk,
- * but not yet sent to the client, and buffer it in the libpq output
+ * Read up to wal_sender_max_send_size bytes of WAL that's been flushed to
+ * disk, but not yet sent to the client, and buffer it in the libpq output
* buffer.
*
* If there is no unsent WAL remaining, WalSndCaughtUp is set to true,
@@ -3076,8 +3068,9 @@ XLogSendPhysical(void)
/*
* Figure out how much to send in one message. If there's no more than
- * MAX_SEND_SIZE bytes to send, send everything. Otherwise send
- * MAX_SEND_SIZE bytes, but round back to logfile or page boundary.
+ * wal_sender_max_send_size bytes to send, send everything. Otherwise send
+ * wal_sender_max_send_size bytes, but round back to logfile or page
+ * boundary.
*
* The rounding is not only for performance reasons. Walreceiver relies on
* the fact that we never split a WAL record across two messages. Since a
@@ -3087,7 +3080,7 @@ XLogSendPhysical(void)
*/
startptr = sentPtr;
endptr = startptr;
- endptr += MAX_SEND_SIZE;
+ endptr += wal_sender_max_send_size;
/* if we went beyond SendRqstPtr, back off */
if (SendRqstPtr <= endptr)
@@ -3106,7 +3099,7 @@ XLogSendPhysical(void)
}
nbytes = endptr - startptr;
- Assert(nbytes <= MAX_SEND_SIZE);
+ Assert(nbytes <= wal_sender_max_send_size);
/*
* OK to read and send the slice.
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 1ad3367159..8d61b006c4 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -617,6 +617,17 @@ check_max_wal_senders(int *newval, void **extra, GucSource source)
return true;
}
+/*
+ * GUC check_hook for wal_sender_max_send_size
+ */
+bool
+check_wal_sender_max_send_size(int *newval, void **extra, GucSource source)
+{
+ if (*newval > wal_segment_size)
+ return false;
+ return true;
+}
+
/*
* Early initialization of a backend (either standalone or under postmaster).
* This happens even before InitPostgres.
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index e53ebc6dc2..0ecd8b8940 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2892,6 +2892,17 @@ struct config_int ConfigureNamesInt[] =
NULL, NULL, NULL
},
+ {
+ {"wal_sender_max_send_size", PGC_SIGHUP, REPLICATION_SENDING,
+ gettext_noop("Sets size of the maximum data payload in a WAL data message."),
+ NULL,
+ GUC_UNIT_BYTE,
+ },
+ &wal_sender_max_send_size,
+ 16 * XLOG_BLCKSZ, XLOG_BLCKSZ, INT_MAX,
+ check_wal_sender_max_send_size, NULL, NULL
+ },
+
{
{"commit_delay", PGC_SUSET, WAL_SETTINGS,
gettext_noop("Sets the delay in microseconds between transaction commit and "
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 835b0e9ba8..bc393245e1 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -323,6 +323,7 @@
#wal_keep_size = 0 # in megabytes; 0 disables
#max_slot_wal_keep_size = -1 # in megabytes; -1 disables
#wal_sender_timeout = 60s # in milliseconds; 0 disables
+#wal_sender_max_send_size = 128kB # in bytes
#track_commit_timestamp = off # collect timestamp of transaction commit
# (change requires restart)
diff --git a/src/include/replication/walsender.h b/src/include/replication/walsender.h
index 1b58d50b3b..f4d3c73f4d 100644
--- a/src/include/replication/walsender.h
+++ b/src/include/replication/walsender.h
@@ -31,6 +31,7 @@ extern PGDLLIMPORT bool wake_wal_senders;
/* user-settable parameters */
extern PGDLLIMPORT int max_wal_senders;
extern PGDLLIMPORT int wal_sender_timeout;
+extern PGDLLIMPORT int wal_sender_max_send_size;
extern PGDLLIMPORT bool log_replication_commands;
extern void InitWalSender(void);
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index 5300c44f3b..b0ac07171c 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -84,6 +84,7 @@ extern bool check_maintenance_io_concurrency(int *newval, void **extra,
extern void assign_maintenance_io_concurrency(int newval, void *extra);
extern bool check_max_connections(int *newval, void **extra, GucSource source);
extern bool check_max_wal_senders(int *newval, void **extra, GucSource source);
+extern bool check_wal_sender_max_send_size(int *newval, void **extra, GucSource source);
extern bool check_max_slot_wal_keep_size(int *newval, void **extra,
GucSource source);
extern void assign_max_wal_size(int newval, void *extra);
--
2.34.1
