Re: [HACKERS] increasing the default WAL segment size

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

General comments:
There was some discussion about the impact of this on small installs, particularly around min_wal_size. The concern was
thatchanging the default segment size to 64MB would significantly increase min_wal_size in terms of bytes. The default
valuefor min_wal_size is 5 segments, so 16MB->64MB would mean going from 80MB to 320MB. IMHO if you're worried about
thatthen just initdb with a smaller segment size. There's probably a number of other changes a small environment wants
tomake besides that. Perhaps it'd be worth making DEFAULT_XLOG_SEG_SIZE a configure option to better support that.
 

It's not clear from the thread that there is consensus that this feature is desired. In particular, the performance
aspectsof changing segment size from a C constant to a variable are in question. Someone with access to large hardware
shouldtest that. Andres[1] and Robert[2] did suggest that the option could be changed to a bitshift, which IMHO would
alsosolve some sanity-checking issues.
 

+     * initdb passes the WAL segment size in an environment variable. We don't
+     * bother doing any sanity checking, we already check in initdb that the
+     * user gives a sane value.

That doesn't seem like a good idea to me. If anything, the backend should sanity-check and initdb just rely on that.
Perhapsthis is how other initdb options work, but it still seems bogus. In particular, verifying the size is a power of
2seems important, as failing that would probably be ReallyBad(tm).
 

The patch also blindly trusts the value read from the control file; I'm not sure if that's standard procedure or not,
butISTM it'd be worth sanity-checking that as well.
 

The patch leaves the base GUC units for min_wal_size and max_wal_size as the # of segments. I'm not sure if that's a
greatidea.
 

+ * convert_unit
+ *
+ * This takes the value in kbytes and then returns value in user-readable format

This function needs a more specific name, such as pretty_print_kb().

+        /* Check if wal_segment_size is in the power of 2 */
+        for (i = 0;; i++, pow2 = pow(2, i))
+            if (pow2 >= wal_segment_size)
+                break;
+
+        if (wal_segment_size != 1 && pow2 > wal_segment_size)
+        {
+            fprintf(stderr, _("%s: WAL segment size must be in the power of 2\n"), progname);
+            exit(1);
+        }

IMHO it'd be better to use the n & (n-1) check detailed at [3].

Actually, there's got to be other places that need to check this, so it'd be nice to just create a function that
verifiesa number is a power of 2.
 

+    if (log_fname != NULL)
+        XLogFromFileName(log_fname, &minXlogTli, &minXlogSegNo);
+

Please add a comment about why XLogFromFileName has to be delayed.
/*
+ * DEFAULT_XLOG_SEG_SIZE is the size of a single WAL file.  This must be a power
+ * of 2 and larger than XLOG_BLCKSZ (preferably, a great deal larger than
+ * XLOG_BLCKSZ).
+ *
+ * Changing DEFAULT_XLOG_SEG_SIZE requires an initdb.
+ */
+#define DEFAULT_XLOG_SEG_SIZE    (16*1024*1024)

That comment isn't really accurate. It would be more useful to explain that DEFAULT_XLOG_SEG_SIZE is the default size
ofa WAL segment used by initdb if a different value isn't specified.
 

1: https://www.postgresql.org/message-id/20161220082847.7t3t6utvxb6m5tfe%40alap3.anarazel.de
2: https://www.postgresql.org/message-id/CA%2BTgmoZTgnL25o68uPBTS6BD37ojD-1y-N88PkP57FzKqwcmmQ%40mail.gmail.com
3: http://stackoverflow.com/questions/108318/whats-the-simplest-way-to-test-whether-a-number-is-a-power-of-2-in-c