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

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.

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

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

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

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

+ * allocating space and reading ControlFile.
s/and/for

+ {"TB", GUC_UNIT_MB, 1024 * 1024},
+ {"GB", GUC_UNIT_MB, 1024},
+ {"MB", GUC_UNIT_MB, 1},
+ {"kB", GUC_UNIT_MB, -1024},
@@ -2235,10 +2231,10 @@ static struct config_int ConfigureNamesInt[] = {"min_wal_size", PGC_SIGHUP, WAL_CHECKPOINTS,
gettext_noop("Setsthe minimum size to shrink the WAL to."), NULL,
 
- GUC_UNIT_XSEGS
+ GUC_UNIT_MB },
- &min_wal_size,
- 5, 2, INT_MAX,
+ &min_wal_size_mb,
+ DEFAULT_MIN_WAL_SEGS * 16, 2, INT_MAX, NULL, NULL, NULL },

@@ -2246,10 +2242,10 @@ static struct config_int ConfigureNamesInt[] = {"max_wal_size", PGC_SIGHUP, WAL_CHECKPOINTS,
gettext_noop("Setsthe WAL size that triggers a checkpoint."), NULL,
 
- GUC_UNIT_XSEGS
+ GUC_UNIT_MB },
- &max_wal_size,
- 64, 2, INT_MAX,
+ &max_wal_size_mb,
+ DEFAULT_MAX_WAL_SEGS * 16, 2, INT_MAX, NULL, assign_max_wal_size, NULL },
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.

I'm yet to review pg_basebackup module and I'll try to finish that ASAP.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com