Re: [HACKERS] increasing the default WAL segment size - Mailing list pgsql-hackers

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