Re: [HACKERS] increasing the default WAL segment size

Hi,

On 2017-01-23 11:35:11 +0530, Beena Emerson wrote:
> Please find attached an updated WIP patch. I have incorporated almost all
> comments. This is to be applied over Robert's patches. I will post
> performance results later on.
>
> 1. shift (>>) and AND (&) operations: The assign hook of wal_segment_size
> sets the WalModMask and WalShiftBit. All the modulo and division operations
> using XLogSegSize has been replaced with these. However, there are many
> preprocessors which divide with XLogSegSize in xlog_internal.h. I have not
> changed these because it would mean I will have to reassign the WalShiftBit
> along with XLogSegSize in all the modules which use these macros. That does
> not seem to be a good idea. Also, this means shift operator can be used
> only in couple of places.

I think it'd be better not to have XLogSegSize anymore. Silently
changing a macros behaviour from being a compile time constant to
something runtime configurable is a bad idea.

> 8. Declaring XLogSegSize: There are 2 internal variables for the same
> parameter. In original code XLOG_SEG_SIZE is defined in the auto-generated
> file src/include/pg_config.h. And xlog_internal.h defines:
>
> #define XLogSegSize     ((uint32) XLOG_SEG_SIZE)
>
> To avoid renaming all parts of code, I made the following change in
> xlog_internal.h
>
> + extern uint32 XLogSegSize;
>
> +#define XLOG_SEG_SIZE XLogSegSize
>
>  would it be better to just use one variable XLogSegSize everywhere. But
> few external modules could be using XLOG_SEG_SIZE. Thoughts?

They'll quite possibly break with configurable size anyway.  So I'd
rather have those broken explicitly.

> +/*
> + * These variables are set in assign_wal_segment_size
> + *
> + * WalModMask: It is an AND mask for XLogSegSize to allow for faster modulo
> + *           operations using it.
> + *
> + * WalShiftBit: It is an shift bit for XLogSegSize to allow for faster
> + *           division operations using it.
> + *
> + * UsableBytesInSegment: It is the number of bytes in a WAL segment usable for
> + *           WAL data.
> + */
> +uint32               WalModMask;
> +static int   UsableBytesInSegment;
> +static int   WalShiftBit;

This could use some editorializing. "Faster modulo operations" isn't an
explaining how/why it's actually being used. Same for WalShiftBit.

>  /*
>   * Private, possibly out-of-date copy of shared LogwrtResult.
> @@ -957,6 +975,7 @@ XLogInsertRecord(XLogRecData *rdata,
>       if (!XLogInsertAllowed())
>               elog(ERROR, "cannot make new WAL entries during recovery");
>
> +
>       /*----------
>        *

Spurious newline change.

>               if (ptr % XLOG_BLCKSZ == SizeOfXLogShortPHD &&
> -                     ptr % XLOG_SEG_SIZE > XLOG_BLCKSZ)
> +                     (ptr & WalModMask) > XLOG_BLCKSZ)
>                       initializedUpto = ptr - SizeOfXLogShortPHD;
>               else if (ptr % XLOG_BLCKSZ == SizeOfXLogLongPHD &&
> -                              ptr % XLOG_SEG_SIZE < XLOG_BLCKSZ)
> +                              (ptr & WalModMask) < XLOG_BLCKSZ)
>                       initializedUpto = ptr - SizeOfXLogLongPHD;
>               else
>                       initializedUpto = ptr;

How about we introduce a XLogSegmentOffset(XLogRecPtr) function like
macro in a first patch?  That'll reduce the amount of change in the
commit actually changing things quite noticeably, and makes it easier to
adjust things later.  I see very little benefit for in-place usage of
either % XLOG_SEG_SIZE or & WalModMask.

> @@ -1794,6 +1813,7 @@ XLogBytePosToRecPtr(uint64 bytepos)
>       uint32          seg_offset;
>       XLogRecPtr      result;
>
> +
>       fullsegs = bytepos / UsableBytesInSegment;
>       bytesleft = bytepos % UsableBytesInSegment;

spurious change.

> @@ -1878,7 +1898,7 @@ XLogRecPtrToBytePos(XLogRecPtr ptr)
>
>       XLByteToSeg(ptr, fullsegs);
>
> -     fullpages = (ptr % XLOG_SEG_SIZE) / XLOG_BLCKSZ;
> +     fullpages = (ptr & WalModMask) / XLOG_BLCKSZ;
>       offset = ptr % XLOG_BLCKSZ;
>
>       if (fullpages == 0)
> @@ -2043,7 +2063,7 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, bool opportunistic)
>               /*
>                * If first page of an XLOG segment file, make it a long header.
>                */
> -             if ((NewPage->xlp_pageaddr % XLogSegSize) == 0)
> +             if ((NewPage->xlp_pageaddr & WalModMask) == 0)
>               {
>                       XLogLongPageHeader NewLongPage = (XLogLongPageHeader) NewPage;
>
> @@ -2095,6 +2115,7 @@ CalculateCheckpointSegments(void)
>        *        number of segments consumed between checkpoints.
>        *-------
>        */
> +
>       target = (double) max_wal_size / (2.0 + CheckPointCompletionTarget);

spurious change.


>  void
> +assign_wal_segment_size(int newval, void *extra)
> +{
> +     /*
> +      * During system initialization, XLogSegSize is not set so we use
> +      * DEFAULT_XLOG_SEG_SIZE instead.
> +      */
> +     int     WalSegSize = (XLogSegSize == 0) ? DEFAULT_XLOG_SEG_SIZE : XLOG_SEG_SIZE;
> +
> +     wal_segment_size = newval;
> +     UsableBytesInSegment = (wal_segment_size * UsableBytesInPage) -
> +                                                (SizeOfXLogLongPHD - SizeOfXLogShortPHD);
> +     WalModMask = WalSegSize - 1;
> +
> +     /* Set the WalShiftBit */
> +     WalShiftBit = 0;
> +     while (WalSegSize > 1)
> +     {
> +             WalSegSize = WalSegSize >> 1;
> +             WalShiftBit++;
> +     }
> +}

Hm. Are GUC hooks a good way to compute the masks?  Interdependent GUCs
are unfortunately not working well, and several GUCs might end up
depending on these.  I think it might be better to assign the variables
somewhere early in StartupXLOG() or such.

> +
> +void
> +assign_min_wal_size(int newval, void *extra)
> +{
> +     /*
> +      * During system initialization, XLogSegSize is not set so we use
> +      * DEFAULT_XLOG_SEG_SIZE instead.
> +      *
> +      * min_wal_size is in kB and XLogSegSize is in bytes and so it is
> +      * converted to kB for the calculation.
> +      */
> +     int     WalSegSize = (XLogSegSize == 0) ? (DEFAULT_XLOG_SEG_SIZE / 1024) :
> +                                                                               (XLOG_SEG_SIZE / 1024);
> +
> +     min_wal_size = newval / WalSegSize;
> +}
> +
> +void
>  assign_max_wal_size(int newval, void *extra)
>  {
> -     max_wal_size = newval;
> +     /*
> +      * During system initialization, XLogSegSize is not set so we use
> +      * DEFAULT_XLOG_SEG_SIZE instead.
> +      *
> +      * max_wal_size is in kB and XLogSegSize is in bytes and so it is
> +      * converted to bytes for the calculation.
> +      */
> +     int     WalSegSize = (XLogSegSize == 0) ? (DEFAULT_XLOG_SEG_SIZE / 1024) :
> +                                                                               (XLOG_SEG_SIZE / 1024);
> +
> +     max_wal_size = newval / WalSegSize;
>       CalculateCheckpointSegments();
>  }

I don't think it's a good idea to have GUCs that are initially set to
the wrong value and such.  How about just storing bytes, and converting
into segments upon use?

> @@ -2135,8 +2205,8 @@ XLOGfileslop(XLogRecPtr PriorRedoPtr)
>        * correspond to. Always recycle enough segments to meet the minimum, and
>        * remove enough segments to stay below the maximum.
>        */
> -     minSegNo = PriorRedoPtr / XLOG_SEG_SIZE + min_wal_size - 1;
> -     maxSegNo = PriorRedoPtr / XLOG_SEG_SIZE + max_wal_size - 1;
> +     minSegNo = (PriorRedoPtr >> WalShiftBit) + min_wal_size - 1;
> +     maxSegNo = (PriorRedoPtr >> WalShiftBit) + max_wal_size - 1;

I think a macro would be good here too (same prerequisite patch as above).

> @@ -4677,8 +4749,18 @@ XLOGShmemSize(void)
>        */
>       if (XLOGbuffers == -1)
>       {
> -             char            buf[32];
> -
> +             /*
> +              * The calculation of XLOGbuffers, requires the now run-time parameter
> +              * XLogSegSize from the ControlFile. The value determined here is
> +              * required to create the shared memory segment. Hence, temporarily
> +              * allocating space and reading ControlFile here.
> +              */

I don't like comments containing things like "the now run-time paramter"
much - they are likely going to still be there in 10 years, and will be
hard to understand.

But anyway, how about we simply remove the "max one segment" boundary
instead? I don't think it's actually very meaningful - several people
posted benchmarks with more than one segment being beneficial.


> diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
> index 31290d3..87efc3c 100644
> --- a/src/bin/pg_basebackup/streamutil.c
> +++ b/src/bin/pg_basebackup/streamutil.c
> @@ -238,6 +238,59 @@ GetConnection(void)
>  }
>
>  /*
> + * Run the SHOW_WAL_SEGMENT_SIZE command to set the XLogSegSize
> + */
> +bool
> +SetXLogSegSize(PGconn *conn)
> +{

I think this is a confusing function name, because it sounds like
you're setting the SegSize remotely or such. I think making it
XLogRecPtr RetrieveXLogSegSize(conn); or such would lead to better code.

> diff --git a/src/bin/pg_resetxlog/pg_resetxlog.c b/src/bin/pg_resetxlog/pg_resetxlog.c
> index 963802e..4ceebdc 100644
> --- a/src/bin/pg_resetxlog/pg_resetxlog.c
> +++ b/src/bin/pg_resetxlog/pg_resetxlog.c
> @@ -57,6 +57,7 @@
>  #include "storage/large_object.h"
>  #include "pg_getopt.h"
>
> +uint32               XLogSegSize;

This seems like a bad idea - having the same local variable both in
frontend and backend programs seems like a recipe for disaster.