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