This started out as a small patch to make pg_controldata use the logging
API instead of printf statements, and then it became a larger patch to
adjust error and warning messages about invalid WAL segment sizes
(IsValidWalSegSize()) across the board. I went through and made the
primary messages more compact and made the detail messages uniform. In
initdb.c and pg_resetwal.c, I use the newish option_parse_int() to
simplify some of the option parsing. For the backend GUC
wal_segment_size, I added a GUC check hook to do the verification
instead of coding it in bootstrap.c. This might be overkill, but that
way the check is in the right place and it becomes more self-documenting.From f5a933aa4ea7980c3df6d74d845a95f2ce0d5153 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Tue, 22 Aug 2023 15:16:28 +0200
Subject: [PATCH] Make error messages about WAL segment size more consistent
In passing, make pg_controldata use the logging API for warning
messages.
---
src/backend/access/transam/xlog.c | 19 +++++++++++---
src/backend/bootstrap/bootstrap.c | 11 +-------
src/backend/utils/misc/guc_tables.c | 2 +-
src/bin/initdb/initdb.c | 25 +++++--------------
src/bin/pg_basebackup/streamutil.c | 5 ++--
src/bin/pg_controldata/pg_controldata.c | 23 ++++++++---------
.../pg_controldata/t/001_pg_controldata.pl | 6 ++---
src/bin/pg_resetwal/Makefile | 2 ++
src/bin/pg_resetwal/pg_resetwal.c | 18 +++++++------
src/bin/pg_rewind/pg_rewind.c | 12 ++++++---
src/bin/pg_waldump/pg_waldump.c | 12 ++++++---
src/include/utils/guc_hooks.h | 1 +
12 files changed, 71 insertions(+), 65 deletions(-)
diff --git a/src/backend/access/transam/xlog.c
b/src/backend/access/transam/xlog.c
index 60c0b7ec3a..f6f8adc72a 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1995,6 +1995,18 @@ assign_checkpoint_completion_target(double newval, void
*extra)
CalculateCheckpointSegments();
}
+bool
+check_wal_segment_size(int *newval, void **extra, GucSource source)
+{
+ if (!IsValidWalSegSize(*newval))
+ {
+ GUC_check_errdetail("The WAL segment size must be a power of
two between 1 MB and 1 GB.");
+ return false;
+ }
+
+ return true;
+}
+
/*
* At a checkpoint, how many WAL segments to recycle as preallocated future
* XLOG segments? Returns the highest segment that should be preallocated.
@@ -4145,10 +4157,11 @@ ReadControlFile(void)
if (!IsValidWalSegSize(wal_segment_size))
ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg_plural("WAL segment size
must be a power of two between 1 MB and 1 GB, but the control file specifies %d
byte",
- "WAL
segment size must be a power of two between 1 MB and 1 GB, but the control file
specifies %d bytes",
+ errmsg_plural("invalid WAL
segment size in control file (%d byte)",
+
"invalid WAL segment size in control file (%d bytes)",
wal_segment_size,
-
wal_segment_size)));
+
wal_segment_size),
+ errdetail("The WAL segment size
must be a power of two between 1 MB and 1 GB.")));
snprintf(wal_segsz_str, sizeof(wal_segsz_str), "%d", wal_segment_size);
SetConfigOption("wal_segment_size", wal_segsz_str, PGC_INTERNAL,
diff --git a/src/backend/bootstrap/bootstrap.c
b/src/backend/bootstrap/bootstrap.c
index 49e956b2c5..c9ed649d0d 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -280,16 +280,7 @@ BootstrapModeMain(int argc, char *argv[], bool check_only)
strlcpy(OutputFileName, optarg, MAXPGPATH);
break;
case 'X':
- {
- int WalSegSz =
strtoul(optarg, NULL, 0);
-
- if (!IsValidWalSegSize(WalSegSz))
- ereport(ERROR,
-
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("-X
requires a power of two value between 1 MB and 1 GB")));
- SetConfigOption("wal_segment_size",
optarg, PGC_INTERNAL,
-
PGC_S_DYNAMIC_DEFAULT);
- }
+ SetConfigOption("wal_segment_size", optarg,
PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
break;
default:
write_stderr("Try \"%s --help\" for more
information.\n",
diff --git a/src/backend/utils/misc/guc_tables.c
b/src/backend/utils/misc/guc_tables.c
index f9dba43b8c..f14656c1e2 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -3166,7 +3166,7 @@ struct config_int ConfigureNamesInt[] =
DEFAULT_XLOG_SEG_SIZE,
WalSegMinSize,
WalSegMaxSize,
- NULL, NULL, NULL
+ check_wal_segment_size, NULL, NULL
},
{
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 3f4167682a..c66467eb95 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -76,6 +76,7 @@
#include "common/restricted_token.h"
#include "common/string.h"
#include "common/username.h"
+#include "fe_utils/option_utils.h"
#include "fe_utils/string_utils.h"
#include "getopt_long.h"
#include "mb/pg_wchar.h"
@@ -163,8 +164,7 @@ static bool sync_only = false;
static bool show_setting = false;
static bool data_checksums = false;
static char *xlog_dir = NULL;
-static char *str_wal_segment_size_mb = NULL;
-static int wal_segment_size_mb;
+static int wal_segment_size_mb = (DEFAULT_XLOG_SEG_SIZE) / (1024 * 1024);
/* internal vars */
@@ -3258,7 +3258,8 @@ main(int argc, char *argv[])
xlog_dir = pg_strdup(optarg);
break;
case 12:
- str_wal_segment_size_mb = pg_strdup(optarg);
+ if (!option_parse_int(optarg, "--wal-segsize",
1, 1024, &wal_segment_size_mb))
+ exit(1);
break;
case 13:
noinstructions = true;
@@ -3348,22 +3349,8 @@ main(int argc, char *argv[])
check_need_password(authmethodlocal, authmethodhost);
- /* set wal segment size */
- if (str_wal_segment_size_mb == NULL)
- wal_segment_size_mb = (DEFAULT_XLOG_SEG_SIZE) / (1024 * 1024);
- else
- {
- char *endptr;
-
- /* check that the argument is a number */
- wal_segment_size_mb = strtol(str_wal_segment_size_mb, &endptr,
10);
-
- /* verify that wal segment size is valid */
- if (endptr == str_wal_segment_size_mb || *endptr != '\0')
- pg_fatal("argument of --wal-segsize must be a number");
- if (!IsValidWalSegSize(wal_segment_size_mb * 1024 * 1024))
- pg_fatal("argument of --wal-segsize must be a power of
2 between 1 and 1024");
- }
+ if (!IsValidWalSegSize(wal_segment_size_mb * 1024 * 1024))
+ pg_fatal("argument of %s must be a power of 2 between 1 and
1024", "--wal-segsize");
get_restricted_token();
diff --git a/src/bin/pg_basebackup/streamutil.c
b/src/bin/pg_basebackup/streamutil.c
index 15514599c4..75ab9e56f3 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -321,10 +321,11 @@ RetrieveWalSegSize(PGconn *conn)
if (!IsValidWalSegSize(WalSegSz))
{
- pg_log_error(ngettext("WAL segment size must be a power of two
between 1 MB and 1 GB, but the remote server reported a value of %d byte",
- "WAL segment size
must be a power of two between 1 MB and 1 GB, but the remote server reported a
value of %d bytes",
+ pg_log_error(ngettext("remote server reported invalid WAL
segment size (%d byte)",
+ "remote server
reported invalid WAL segment size (%d bytes)",
WalSegSz),
WalSegSz);
+ pg_log_error_detail("The WAL segment size must be a power of
two between 1 MB and 1 GB.");
return false;
}
diff --git a/src/bin/pg_controldata/pg_controldata.c
b/src/bin/pg_controldata/pg_controldata.c
index c390ec51ce..93e0837947 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -167,24 +167,23 @@ main(int argc, char *argv[])
/* get a copy of the control file */
ControlFile = get_controlfile(DataDir, &crc_ok);
if (!crc_ok)
- printf(_("WARNING: Calculated CRC checksum does not match value
stored in file.\n"
- "Either the file is corrupt, or it has a
different layout than this program\n"
- "is expecting. The results below are
untrustworthy.\n\n"));
+ {
+ pg_log_warning("calculated CRC checksum does not match value
stored in control file");
+ pg_log_warning_detail("Either the control file is corrupt, or
it has a different layout than this program "
+ "is expecting. The
results below are untrustworthy.");
+ }
/* set wal segment size */
WalSegSz = ControlFile->xlog_seg_size;
if (!IsValidWalSegSize(WalSegSz))
{
- printf(_("WARNING: invalid WAL segment size\n"));
- printf(ngettext("The WAL segment size stored in the file, %d
byte, is not a power of two\n"
- "between 1 MB and 1 GB. The
file is corrupt and the results below are\n"
- "untrustworthy.\n\n",
- "The WAL segment size stored in
the file, %d bytes, is not a power of two\n"
- "between 1 MB and 1 GB. The
file is corrupt and the results below are\n"
- "untrustworthy.\n\n",
- WalSegSz),
- WalSegSz);
+ pg_log_warning(ngettext("invalid WAL segment size in control
file (%d byte)",
+ "invalid WAL
segment size in control file (%d bytes)",
+ WalSegSz),
+ WalSegSz);
+ pg_log_warning_detail("The WAL segment size must be a power of
two between 1 MB and 1 GB.");
+ pg_log_warning_detail("The file is corrupt and the results
below are untrustworthy.");
}
/*
diff --git a/src/bin/pg_controldata/t/001_pg_controldata.pl
b/src/bin/pg_controldata/t/001_pg_controldata.pl
index 0c641036e9..4918ea8944 100644
--- a/src/bin/pg_controldata/t/001_pg_controldata.pl
+++ b/src/bin/pg_controldata/t/001_pg_controldata.pl
@@ -36,11 +36,11 @@
command_checks_all(
[ 'pg_controldata', $node->data_dir ],
0,
+ [qr/./],
[
- qr/WARNING: Calculated CRC checksum does not match value stored
in file/,
- qr/WARNING: invalid WAL segment size/
+ qr/warning: calculated CRC checksum does not match value stored
in control file/,
+ qr/warning: invalid WAL segment size/
],
- [qr/^$/],
'pg_controldata with corrupted pg_control');
done_testing();
diff --git a/src/bin/pg_resetwal/Makefile b/src/bin/pg_resetwal/Makefile
index a363b948b5..5c86435e22 100644
--- a/src/bin/pg_resetwal/Makefile
+++ b/src/bin/pg_resetwal/Makefile
@@ -15,6 +15,8 @@ subdir = src/bin/pg_resetwal
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
+LDFLAGS_INTERNAL += -L$(top_builddir)/src/fe_utils -lpgfeutils
+
OBJS = \
$(WIN32RES) \
pg_resetwal.o
diff --git a/src/bin/pg_resetwal/pg_resetwal.c
b/src/bin/pg_resetwal/pg_resetwal.c
index e7ef2b8bd0..9bebc2a995 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -55,6 +55,7 @@
#include "common/logging.h"
#include "common/restricted_token.h"
#include "common/string.h"
+#include "fe_utils/option_utils.h"
#include "getopt_long.h"
#include "pg_getopt.h"
#include "storage/large_object.h"
@@ -290,13 +291,16 @@ main(int argc, char *argv[])
break;
case 1:
- errno = 0;
- set_wal_segsize = strtol(optarg, &endptr, 10) *
1024 * 1024;
- if (endptr == optarg || *endptr != '\0' ||
errno != 0)
- pg_fatal("argument of --wal-segsize
must be a number");
- if (!IsValidWalSegSize(set_wal_segsize))
- pg_fatal("argument of --wal-segsize
must be a power of 2 between 1 and 1024");
- break;
+ {
+ int wal_segsize_mb;
+
+ if (!option_parse_int(optarg,
"--wal-segsize", 1, 1024, &wal_segsize_mb))
+ exit(1);
+ set_wal_segsize = wal_segsize_mb * 1024
* 1024;
+ if (!IsValidWalSegSize(set_wal_segsize))
+ pg_fatal("argument of %s must
be a power of 2 between 1 and 1024", "--wal-segsize");
+ break;
+ }
default:
/* getopt_long already emitted a complaint */
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index f7f3b8227f..7f69f02441 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -1023,10 +1023,14 @@ digestControlFile(ControlFileData *ControlFile, const
char *content,
WalSegSz = ControlFile->xlog_seg_size;
if (!IsValidWalSegSize(WalSegSz))
- pg_fatal(ngettext("WAL segment size must be a power of two
between 1 MB and 1 GB, but the control file specifies %d byte",
- "WAL segment size must be a
power of two between 1 MB and 1 GB, but the control file specifies %d bytes",
- WalSegSz),
- WalSegSz);
+ {
+ pg_log_error(ngettext("invalid WAL segment size in control file
(%d byte)",
+ "invalid WAL segment
size in control file (%d bytes)",
+ WalSegSz),
+ WalSegSz);
+ pg_log_error_detail("The WAL segment size must be a power of
two between 1 MB and 1 GB.");
+ exit(1);
+ }
/* Additional checks on control file */
checkControlFile(ControlFile);
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index e8b5a6cd61..b9acfed3b7 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -252,10 +252,14 @@ search_directory(const char *directory, const char *fname)
WalSegSz = longhdr->xlp_seg_size;
if (!IsValidWalSegSize(WalSegSz))
- pg_fatal(ngettext("WAL segment size must be a
power of two between 1 MB and 1 GB, but the WAL file \"%s\" header specifies %d
byte",
- "WAL segment
size must be a power of two between 1 MB and 1 GB, but the WAL file \"%s\"
header specifies %d bytes",
- WalSegSz),
- fname, WalSegSz);
+ {
+ pg_log_error(ngettext("invalid WAL segment size
in WAL file \"%s\" (%d byte)",
+
"invalid WAL segment size in WAL file \"%s\" (%d bytes)",
+
WalSegSz),
+ fname, WalSegSz);
+ pg_log_error_detail("The WAL segment size must
be a power of two between 1 MB and 1 GB.");
+ exit(1);
+ }
}
else if (r < 0)
pg_fatal("could not read file \"%s\": %m",
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index 2ecb9fc086..5d8b2c981b 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -156,6 +156,7 @@ extern bool check_wal_buffers(int *newval, void **extra,
GucSource source);
extern bool check_wal_consistency_checking(char **newval, void **extra,
GucSource source);
extern void assign_wal_consistency_checking(const char *newval, void *extra);
+extern bool check_wal_segment_size(int *newval, void **extra, GucSource
source);
extern void assign_xlog_sync_method(int new_sync_method, void *extra);
extern bool check_io_direct(char **newval, void **extra, GucSource source);
extern void assign_io_direct(const char *newval, void *extra);
--
2.41.0
