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 that changing the default
segment size to 64MB would significantly increase min_wal_size in terms of
bytes. The default value for min_wal_size is 5 segments, so 16MB->64MB would
mean going from 80MB to 320MB. IMHO if you're worried about that then just
initdb with a smaller segment size. There's probably a number of other changes
a small environment wants to make 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 aspects of changing segment size from a
C constant to a variable are in question. Someone with access to large hardware
should test that. Andres[1] and Robert[2] did suggest that the option could be
changed to a bitshift, which IMHO would also solve 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. Perhaps this is how other initdb
options work, but it still seems bogus. In particular, verifying the size is a
power of 2 seems 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, but ISTM 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 great idea.
+ * 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 verifies a 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 of a 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
