Hello, Thank you for your review.
On Tue, Jan 3, 2017 at 2:53 AM, Jim Nasby <jim.na...@bluetreble.com> wrote: > 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. > The patch maintains the current XLOG_SEG_SIZE of 16MB as the default. Only the capability to change its value has been moved for configure to initdb. > > 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. > There is a CRC check to detect error in the file. I think all the ControlFile values are used directly and not re-verified. > > 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. > I think we can leave it as is. This is used in CalculateCheckpontSegments and in XLOGfileslop to calculate the segment numbers. > + * 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(). > I agree pretty_print_kb would have been a better for this function. However, I have realised that using the show hook and this function is not suitable and have found a better way of handling the removal of GUC_UNIT_XSEGS which no longer needs this function : using the GUC_UNIT_KB, convert the value in bytes to wal segment count instead in the assign hook. The next version of patch will use this. > > + /* 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]. > Yes, even I had come across it. I will incorporate this in the next version of the patch. > > 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. > Oh yes!. > > /* > + * 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. > I will correct this comment The new version of the patch will be posted soon. Thank you, Beena Emerson Have a Great Day!