Re: increasing the default WAL segment size - Mailing list pgsql-hackers

On Wed, Mar 22, 2017 at 9:41 PM, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:

On Wed, Mar 22, 2017 at 3:14 PM, Beena Emerson <memissemerson@gmail.com> wrote:
> PFA an updated patch which fixes a minor bug I found. It only increases the
> string size in pretty_wal_size function.
> The 01-add-XLogSegmentOffset-macro.patch has also been rebased.
Thanks for the updated versions. Here is a partial review of the patch:

In pg_standby.c and pg_waldump.c,
+ XLogPageHeader hdr = (XLogPageHeader) buf;
+ XLogLongPageHeader NewLongPage = (XLogLongPageHeader) hdr;
+
+ XLogSegSize = NewLongPage->xlp_seg_size;
It waits until the file is at least XLOG_BLCKSZ, then gets the
expected final size from XLogPageHeader. This looks really clean
compared to the previous approach.

thank you for testing. PFA the rebased patch incorporating your comments.

+ * Verify that the min and max wal_size meet the minimum requirements.
Better to write min_wal_size and max_wal_size.

Updated wherever applicable.

+ errmsg("Insufficient value for \"min_wal_size\"")));
"min_wal_size %d is too low" may be? Use lower case for error
messages. Same for max_wal_size.

updated.

+ /* Set the XLogSegSize */
+ XLogSegSize = ControlFile->xlog_seg_size;
+
A call to IsValidXLogSegSize() will be good after this, no?

Is it necessary? We already have the CRC check for ControlFiles. Is that not enough?

+ /* Update variables using XLogSegSize */
+ check_wal_size();
The method name looks somewhat misleading compared to the comment for
it, doesn't it?

The method name is correct since it only checks if the value provided is sufficient (at least 2 segment size). I have updated the comment to say Check and update variables dependent on XLogSegSize

This patch introduces a new guc_unit having values in MB for
max_wal_size and min_wal_size. I'm not sure about the upper limit
which is set to INT_MAX for 32-bit systems as well. Is it needed to
define something like MAX_MEGABYTES similar to MAX_KILOBYTES?
It is worth mentioning that GUC_UNIT_KB can't be used in this case
since MAX_KILOBYTES is INT_MAX / 1024(<2GB) on 32-bit systems. That's
not a sufficient value for min_wal_size/max_wal_size.

While testing with pg_waldump, I got the following error.
bin/pg_waldump -p master/pg_wal/ -s 0/01000000
Floating point exception (core dumped)
Stack:
#0  0x00000000004039d6 in ReadPageInternal ()
#1  0x0000000000404c84 in XLogFindNextRecord ()
#2  0x0000000000401e08 in main ()
I think that the problem is in following code:
/* parse files as start/end boundaries, extract path if not specified */
    if (optind < argc)
{
....
+ if (!RetrieveXLogSegSize(full_path))
...
}
In this case, RetrieveXLogSegSize is conditionally called. So, if the
condition is false, XLogSegSize will not be initialized.