Hello, PFA the updated patch.
On Fri, Mar 17, 2017 at 6:40 AM, Robert Haas <robertmh...@gmail.com> wrote:
> On Tue, Mar 14, 2017 at 1:44 AM, Beena Emerson <memissemer...@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
02-initdb-walsegsize-v5.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
