Re: increasing the default WAL segment size

PFA the updated patches.

On Wed, Aug 16, 2017 at 1:55 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> Hi,
>
> Personally I find the split between 03 and 04 and their naming a bit
> confusing. I'd rather just merge them. These patches need a rebase,
> they don't apply anymore.

01 is rebased. 04 and 03 are now merged into
02-initdb-configurable-walsegsize.patch. It also fixes a issue on
Windows, the XLogSegSize is now passed through BackendParameters so
the values are available during process forking.

>
>
> On 2017-07-06 12:04:12 +0530, Beena Emerson wrote:
>> @@ -4813,6 +4836,18 @@ XLOGShmemSize(void)
>> {
>> char buf[32];
>>
>> + /*
>> + * The calculation of XLOGbuffers requires the run-time parameter
>> + * XLogSegSize which is set from the control file. This value is
>> + * required to create the shared memory segment. Hence, temporarily
>> + * allocate space for reading the control file.
>> + */
>
> This makes me uncomfortable. Having to choose the control file multiple
> times seems wrong. We're effectively treating the control file as part
> of the configuration now, and that means we should move it's parsing to
> an earlier part of startup.

Yes, this may seem ugly. ControlFile was originally read into the
shared memory segment but then we now need the XLogSegSize from the
ControlFile to initialise the shared memory segment. I could not
figure out any other way to achieve this.

>
>
>> + if (!IsBootstrapProcessingMode())
>> + {
>> + ControlFile = palloc(sizeof(ControlFileData));
>> + ReadControlFile();
>> + pfree(ControlFile);
>
> General plea: Please reset to NULL in cases like this, otherwise the
> pointer will [temporarily] point into a freed memory location, which
> makes debugging harder.

done.

>
>
>
>> @@ -8146,6 +8181,9 @@ InitXLOGAccess(void)
>> ThisTimeLineID = XLogCtl->ThisTimeLineID;
>> Assert(ThisTimeLineID != 0 || IsBootstrapProcessingMode());
>>
>> + /* set XLogSegSize */
>> + XLogSegSize = ControlFile->xlog_seg_size;
>> +
>
> Hm, why do we have two variables keeping track of the segment size?
> wal_segment_size and XLogSegSize? That's bound to lead to confusion.
>

wal_segment_size is the guc which stores the number of segments
(XLogSegSize / XLOG_BLCKSZ).

>
>> /* Use GetRedoRecPtr to copy the RedoRecPtr safely */
>> (void) GetRedoRecPtr();
>> /* Also update our copy of doPageWrites. */
>> diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
>> index b3f0b3c..d2c524b 100644
>> --- a/src/backend/bootstrap/bootstrap.c
>> +++ b/src/backend/bootstrap/bootstrap.c
>> @@ -19,6 +19,7 @@
>>
>> #include "access/htup_details.h"
>> #include "access/xact.h"
>> +#include "access/xlog_internal.h"
>> #include "bootstrap/bootstrap.h"
>> #include "catalog/index.h"
>> #include "catalog/pg_collation.h"
>> @@ -47,6 +48,7 @@
>> #include "utils/tqual.h"
>>
>> uint32 bootstrap_data_checksum_version = 0; /* No checksum */
>> +uint32 XLogSegSize;
>
> Se we define the same variable declared in a header in multiple files
> (once for each applicationq)? I'm pretty strongly against that. Why's
> that needed/a good idea? Neither is it clear to me why the definition
> was moved from xlog.c to bootstrap.c? That doesn't sound like a natural
> place.

I have moved back to xlog.c.

>
>
>> /*
>> + * Calculate the default wal_size in proper unit.
>> + */
>> +static char *
>> +pretty_wal_size(int segment_count)
>> +{
>> + double val = wal_segment_size / (1024 * 1024) * segment_count;
>> + double temp_val;
>> + char *result = malloc(10);
>> +
>> + /*
>> + * Wal segment size ranges from 1MB to 1GB and the default
>> + * segment_count is 5 for min_wal_size and 64 for max_wal_size, so the
>> + * final values can range from 5MB to 64GB.
>> + */
>
> Referencing the defaults here seems unnecessary. And nearly a guarantee
> that the values in the comment will go out of date soon-ish.

Removed the comment.

>
>
>> + /* set default max_wal_size and min_wal_size */
>> + snprintf(repltok, sizeof(repltok), "min_wal_size = %s",
>> + pretty_wal_size(DEFAULT_MIN_WAL_SEGS));
>> + conflines = replace_token(conflines, "#min_wal_size = 80MB", repltok);
>> +
>> + snprintf(repltok, sizeof(repltok), "max_wal_size = %s",
>> + pretty_wal_size(DEFAULT_MAX_WAL_SEGS));
>> + conflines = replace_token(conflines, "#max_wal_size = 1GB", repltok);
>> +
>
> Hm. So postgresql.conf.sample values are now going to contain misleading
> information for clusters with non-default segment sizes.
>
> Have we discussed instead defaulting min_wal_size/max_wal_size to a
> constant amount of megabytes and rounding up when it doesn't work for
> a particular segment size?

This was not discussed.

In the original code, the min_wal_size and max_wal_size are computed
in the guc.c for any wal_segment_size set at configure.

{
{"min_wal_size", PGC_SIGHUP, WAL_CHECKPOINTS,
gettext_noop("Sets the minimum size to shrink the WAL to."),
NULL,
GUC_UNIT_MB
},
&min_wal_size_mb,
5 * (XLOG_SEG_SIZE / (1024 * 1024)), 2, MAX_KILOBYTES,
NULL, NULL, NULL
},

{
{"max_wal_size", PGC_SIGHUP, WAL_CHECKPOINTS,
gettext_noop("Sets the WAL size that triggers a checkpoint."),
NULL,
GUC_UNIT_MB
},
&max_wal_size_mb,
64 * (XLOG_SEG_SIZE / (1024 * 1024)), 2, MAX_KILOBYTES,
NULL, assign_max_wal_size, NULL
},

Hence I have retained the same calculation for min_wal_size and
max_wal_size. If we get consensus for fixing a default and updating
when required, then I will change the code accordingly.

>
>
>> diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
>> index 9c0039c..c805f12 100644
>> --- a/src/include/access/xlog_internal.h
>> +++ b/src/include/access/xlog_internal.h
>> @@ -91,6 +91,11 @@ typedef XLogLongPageHeaderData *XLogLongPageHeader;
>> */
>>
>> extern uint32 XLogSegSize;
>> +#define XLOG_SEG_SIZE XLogSegSize
>
> I don't think this is a good idea, we should rather rip the bandaid
> of and remove this macro. If people are assuming it's a macro they'll
> just run into more confusing errors/problems.
>

Okay. done.

>
>> diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
>> index f3b3529..f31c30e 100644
>> --- a/src/include/pg_config_manual.h
>> +++ b/src/include/pg_config_manual.h
>> @@ -14,6 +14,12 @@
>> */
>>
>> /*
>> + * This is default value for WAL_segment_size to be used at intidb when run
>> + * without --walsegsize option.
>> + */
>
> WAL_segment_size is a bit weirdly cased...

corrected.

>
>
>> diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
>> index d7fa2a8..279728d 100644
>> --- a/contrib/pg_standby/pg_standby.c
>> +++ b/contrib/pg_standby/pg_standby.c
>> @@ -33,9 +33,12 @@
>> #include "pg_getopt.h"
>>
>> #include "access/xlog_internal.h"
>> +#include "access/xlogreader.h"
>>
>> const char *progname;
>>
>> +uint32 XLogSegSize;
>> +
>> /* Options and defaults */
>> int sleeptime = 5; /* amount of time to sleep between file checks */
>> int waittime = -1; /* how long we have been waiting, -1 no wait
>> @@ -100,6 +103,72 @@ int nextWALFileType;
>>
>> struct stat stat_buf;
>>
>> +static bool SetWALFileNameForCleanup(void);
>> +
>> +/* Set XLogSegSize from the WAL file specified by WALFilePath */
>
> Hm. Why don't we instead accept the segment size as a parameter expanded
> in restore_command? Then this magic isn't necessary. This won't be the
> only command needing it.
>
>
>> +static bool
>> +RetrieveXLogSegSize()
>
> Please add void as argument.

done.

>> -#define MaxSegmentsPerLogFile ( 0xFFFFFFFF / XLOG_SEG_SIZE )
>> -
>> static void
>> CustomizableCleanupPriorWALFiles(void)
>> {
>> @@ -315,6 +384,7 @@ SetWALFileNameForCleanup(void)
>> uint32 log_diff = 0,
>> seg_diff = 0;
>> bool cleanup = false;
>> + int MaxSegmentsPerLogFile = (0xFFFFFFFF / XLogSegSize);
>
> Inconsistent variable naming here.

XLOG_SEG_SIZE is now removed so we can only use the variable XLogSegSize.

>
>> /*
>> + * From version 10, explicitly set XLogSegSize using SHOW wal_segment_size
>> + * since ControlFile is not accessible here.
>> + */
>> +bool
>> +RetrieveXLogSegSize(PGconn *conn)
>> +{
>
>> + /* wal_segment_size ranges from 1MB to 1GB */
>> + tmp_result = pg_strdup(PQgetvalue(res, 0, 0));
>
> Why strdup if we just do a sscanf?

Fixed.

>
>> +/*
>> + * Try to find fname in the given directory. Returns true if it is found,
>> + * false otherwise. If fname is NULL, search the complete directory for any
>> + * file with a valid WAL file name.
>> + */
>> +static bool
>> +search_directory(char *directory, char *fname)
>
> This doesn't mention an important fact, namely that this routine tries
> to figure out XLogSegSize from the file...

Added to the comment.

>
> This is kind of an ugly approach, but I don't see anything really simpler.
>
>> +/* check that the given size is a valid XLogSegSize */
>> +#define IsPowerOf2(x) (((x) & ((x)-1)) == 0)
>
> Not that it really matters here, but this isn't correct for 0 I
> believe.
>
>> +#define IsValidXLogSegSize(size) \
>> + (IsPowerOf2(size) && \
>> + (size >= XLogSegMinSize && size <= XLogSegMaxSize))
>> +
>
> Please wrap references to size in parens.

Added the parens.

>
> Should we consider making this an inline function instead? There's
> some multiple evaluation hazard here...
>
>
>> +#define XLogSegmentsPerXLogId (UINT64CONST(0x100000000) / XLogSegSize)
>>
>> #define XLogSegNoOffsetToRecPtr(segno, offset, dest) \
>> - (dest) = (segno) * XLOG_SEG_SIZE + (offset)
>> + (dest) = (segno) * XLogSegSize + (offset)
>
> I don't think it's a good idea to implicitly reference a global variable
> in such a macro. IOW, I think this needs to grow another parameter, and
> callers should get adjusted. I know this'll affect a number of macros,
> but it still seems like the right thing to do. I'd welcome other
> opinions on this.

I have added this change in the separate patch
(03-modify-xlog-macros.patch) since it touches a lot of files.

--

Beena Emerson

EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company