Re: increasing the default WAL segment size

Hello,

PFA the updated patch.

On Fri, Mar 17, 2017 at 6:40 AM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Tue, Mar 14, 2017 at 1:44 AM, Beena Emerson <memissemerson@gmail.com>
> wrote:
> > Attached is the updated patch. It fixes the issues and also updates few
> code
> > comments.
>
> I did an initial readthrough of this patch tonight just to get a
> feeling for what's going on.  Based on that, here are a few review
> comments:
>
> The changes to pg_standby seem to completely break the logic to wait
> until the file has attained the correct size.  I don't know how to
> salvage that logic off-hand, but just breaking it isn't acceptable.
>

Using, the XLogLongPageHeader->xlp_seg_size, all the original checks have
been retained. This methid is even used in pg_waldump.



>
> +         Note that changing this value requires an initdb.
>
> Instead, maybe say something like "Note that this value is fixed for
> the lifetime of the database cluster."
>

Corrected.


>
> -int            max_wal_size = 64;    /* 1 GB */
> -int            min_wal_size = 5;    /* 80 MB */
> +int            wal_segment_size = 2048;    /* 16 MB */
> +int            max_wal_size = 1024 * 1024;    /* 1 GB */
> +int            min_wal_size = 80 * 1024;    /* 80 MB */
>
> If wal_segment_size is now measured in multiple of XLOG_BLCKSZ, then
> it's not the case that 2048 is always 16MB.  If the other values are
> now measured in kB, perhaps rename the variables to add _kb, to avoid
> confusion with the way it used to work (and in general).  The problem
> with leaving this as-is is that any existing references to
> max_wal_size in core or extension code will silently break; you want

it to break in a noticeable way so that it gets fixed.
>
>
The  wal_segment_size  now is DEFAULT_XLOG_SEG_SIZE / XLOG_BLCKSZ;
min and max wal_size  have _kb postfix


> + * UsableBytesInSegment: It is set in assign_wal_segment_size and stores
> the
> + *         number of bytes in a WAL segment usable for WAL data.
>
> The comment doesn't need to say where it gets set, and it doesn't need
> to repeat the variable name.  Just say "The number of bytes in a..."
>

Done.


>
> +assign_wal_segment_size(int newval, void *extra)
>
> Why does a PGC_INTERNAL GUC need an assign hook?  I think the GUC
> should only be there to expose the value; it shouldn't have
> calculation logic associated with it.
>

Removed the function and called the functions in ReadControlFile.


>
>      /*
> +     * 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.
> +     */
> +    XLogSegSize = pg_atoi(getenv("XLOG_SEG_SIZE"), sizeof(uint32), 0);
>
> I think we should bother.  I don't like the idea of the postmaster
> crashing in flames without so much as a reasonable error message if
> this parameter-passing mechanism goes wrong.
>

I have rechecked the XLogSegSize.


>
> +        {"wal-segsize", required_argument, NULL, 'Z'},
>
> When adding an option with no documented short form, generally one
> picks a number that isn't a character for the value at the end.  See
> pg_regress.c or initdb.c for examples.
>

Done.


>
> +               wal_segment_size = atoi(str_wal_segment_size);
>
> So, you're comfortable interpreting --wal-segsize=1TB or
> --wal-segsize=1GB as 1?  Implicitly, 1MB?
>

Imitating the current behaviour of config option --with-wal-segment, I have
used strtol to throw an error if the value is not only integers.


>
> + * ControlFile is not accessible here so use SHOW wal_segment_size command
> + * to set the XLogSegSize
>
> Breaks compatibility with pre-9.6 servers.
>

Added check for the version, the SHOW command will be run only in v10 and
above. Previous versions do not need this.


>
> --
Thank you,

Beena Emerson

EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company