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


Greetings,

Andres Freund