Re: [HACKERS] increasing the default WAL segment size

On 1/5/17 5:38 AM, Beena Emerson wrote:
> On Tue, Jan 3, 2017 at 2:53 AM, Jim Nasby <Jim.Nasby@bluetreble.com
> <mailto:Jim.Nasby@bluetreble.com>> wrote:
>     General comments:
>     There was some discussion about the impact of this on small
>     installs, particularly around min_wal_size. The concern was 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.

Ah, I missed that. Thanks for clarifying.

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

Are you going to change to a bitshift in the next patch?

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

Sounds good. I do still think the variable from initdb should be 
sanity-checked.

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

My concern here is that we just changed from segments to KB for all the 
checkpoint settings, and this is introducing segments back in, but ...

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

... it sounds like you're going back to exposing KB to users, and that's 
all that really matters.

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

See my other email about that.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)