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

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.


Greetings,

Andres Freund