Re: increasing the default WAL segment size

Hello Andres,

Thank you for your review.

On Fri, Jan 27, 2017 at 12:39 AM, Andres Freund <andres@anarazel.de> wrote:

> 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.
>

I dont think I understood u clearly. You mean convert the macros using
XLogSegSize to functions?


> > 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.
>

Ok. I will remove the XLOG_SEGS_SIZE variable then?


> > +/*
> > + * 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.
>

 I will change these comments.


>
> >  /*
> >   * 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.
>

I will check this.


>
>
> > @@ -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.
>

I am not sure about these interdependent GUCs. I need to study this better
and make changes as required.


> > +
> > +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?
>

max_wal_size is used in CalculateCheckpointSegments and XLOGfileslop.
min_wal_size is used in XLOGfileslop only.

XLOGfileslop is called after the postgres has started up and would have
XLogSegSize set by then but CalculateCheckpointSegments  would be a
problem. assign_max_wal_size calls CalculateCheckpointSegments which will
need the value as segment count not bytes. If we continue as bytes, then we
will need to shift the WalSegSize adjustment in the
CalculateCheckpointSegments.




> > @@ -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.
>

you are right.


>
> 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.
>

I agree. I will do the needful.


>
> > 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.
>

I had to use the same variable name because they were used in the macros
specified in the xlog_internal.h,  So re-assigning this variable would
automatically make the macros using XLogSegSize accessible in these
programs. Else they will throw error "undefined reference to `XLogSegSize'"


-- 
Thank you,

Beena Emerson

Have a Great Day!