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!
