Re: increasing the default WAL segment size

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.

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.

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

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

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

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

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

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

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

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

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

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

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

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.

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.

Greetings,

Andres Freund