Re: increasing the default WAL segment size

PFA an updated patch.

This fixes an issue reported by Tushar internally. Since the patch changes
the way min and max wal_size is stored internally from segment count to
size in kb, it limited the maximum size of min and max_wal_size to 2GB in
32 bit systems.

The minimum required segment is 2 and the minimum wal size is 1MB. So the
lowest possible value of the min/max wal_size is 2MB. Hence, I have changed
the internal representation to MB instead of KB so that we can increase the
range. Also, for any wal-seg-size, it retains the default seg count as 5
and 64 for min and max wal_size. Consequently, the size of the variables
increase automatically according to the wal_segment_size. This behaviour is
similar to that of existing code.

I have also run pg_indent on the files.


On Mon, Mar 20, 2017 at 11:37 PM, Beena Emerson <memissemerson@gmail.com>
wrote:

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



-- 
Thank you,

Beena Emerson

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