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(at)gmail(dot)com>
wrote:

> Hello,
>
> PFA the updated patch.
>
> On Fri, Mar 17, 2017 at 6:40 AM, Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
>
>> On Tue, Mar 14, 2017 at 1:44 AM, Beena Emerson <memissemerson(at)gmail(dot)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