Re: 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(dot)Nasby(at)bluetreble(dot)com
> <mailto:Jim(dot)Nasby(at)bluetreble(dot)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)