On Fri, Oct 28, 2022 at 11:48:13AM +0900, Michael Paquier wrote: > Thanks. I have not looked at the checkup logic yet, but the central > declarations seem rather sane, and I have a few comments about the > latter.
So, I've had the energy to look at the check logic today, and noticed that, while the proposed patch is doing the job when loading the in-core GUCs, nothing is happening for the custom GUCs that could be loaded through shared_preload_libraries or just from a LOAD command. After adding an extra check in define_custom_variable() (reworking a bit the interface proposed while on it), I have found a few more issues than what's been already found on this thread: - 5 missing spots in pg_stat_statements. - 3 float rounding issues in pg_trgm. - 1 spot in pg_prewarm. - A few more that had no initialization, but these had a default of false/0/0.0 so it does not influence the end result but I have added some initializations anyway. With all that addressed, I am finishing with the attached. I have added some comments for the default definitions depending on the CFLAGS, explaining the reasons behind the choices made. The CI has produced a green run, which is not the same as the buildfarm, still gives some confidence. Thoughts? -- Michael
From 3f742d6433d88895720f6801c59213f0f4ad17f9 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Fri, 28 Oct 2022 16:03:08 +0900
Subject: [PATCH v7] GUC C variable sanity check
Added a function to perform a sanity-check comparison of the C variable initial
value with the compiled-in default (boot_val). The purpose of this is to prevent
anybody reading those C declarations from being fooled by mismatched values.
Also fixed some existing mismatching values.
---
src/include/storage/bufmgr.h | 9 ++
src/include/utils/ps_status.h | 6 ++
src/backend/access/transam/xact.c | 2 +-
src/backend/access/transam/xlog.c | 2 +-
src/backend/libpq/be-secure.c | 4 +-
src/backend/postmaster/postmaster.c | 2 +-
src/backend/storage/buffer/bufmgr.c | 10 +--
src/backend/storage/ipc/dsm_impl.c | 2 +-
src/backend/utils/adt/xml.c | 4 +-
src/backend/utils/cache/plancache.c | 2 +-
src/backend/utils/error/elog.c | 2 +-
src/backend/utils/init/globals.c | 4 +-
src/backend/utils/misc/guc.c | 89 +++++++++++++++++++
src/backend/utils/misc/guc_tables.c | 50 +++++------
src/backend/utils/misc/ps_status.c | 3 +-
contrib/auth_delay/auth_delay.c | 2 +-
contrib/pg_prewarm/autoprewarm.c | 2 +-
.../pg_stat_statements/pg_stat_statements.c | 10 +--
contrib/pg_trgm/trgm_op.c | 6 +-
contrib/sepgsql/hooks.c | 4 +-
20 files changed, 157 insertions(+), 58 deletions(-)
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 6f4dfa0960..a1e933f62e 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -69,6 +69,15 @@ extern PGDLLIMPORT bool zero_damaged_pages;
extern PGDLLIMPORT int bgwriter_lru_maxpages;
extern PGDLLIMPORT double bgwriter_lru_multiplier;
extern PGDLLIMPORT bool track_io_timing;
+
+/* effective when prefetching is available */
+#ifdef USE_PREFETCH
+#define DEFAULT_EFFECTIVE_IO_CONCURRENCY 1
+#define DEFAULT_MAINTENANCE_IO_CONCURRENCY 10
+#else
+#define DEFAULT_EFFECTIVE_IO_CONCURRENCY 0
+#define DEFAULT_MAINTENANCE_IO_CONCURRENCY 0
+#endif
extern PGDLLIMPORT int effective_io_concurrency;
extern PGDLLIMPORT int maintenance_io_concurrency;
diff --git a/src/include/utils/ps_status.h b/src/include/utils/ps_status.h
index bba463591f..3bf212e3fd 100644
--- a/src/include/utils/ps_status.h
+++ b/src/include/utils/ps_status.h
@@ -12,6 +12,12 @@
#ifndef PS_STATUS_H
#define PS_STATUS_H
+/* Disabled on Windows as the performance overhead can be significant */
+#ifdef WIN32
+#define DEFAULT_UPDATE_PROCESS_TITLE false
+#else
+#define DEFAULT_UPDATE_PROCESS_TITLE true
+#endif
extern PGDLLIMPORT bool update_process_title;
extern char **save_ps_display_args(int argc, char **argv);
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index fd5103a78e..f45c95faf6 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -75,7 +75,7 @@
* User-tweakable parameters
*/
int DefaultXactIsoLevel = XACT_READ_COMMITTED;
-int XactIsoLevel;
+int XactIsoLevel = XACT_READ_COMMITTED;
bool DefaultXactReadOnly = false;
bool XactReadOnly;
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 8f10effe3a..be54c23187 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -131,7 +131,7 @@ bool wal_init_zero = true;
bool wal_recycle = true;
bool log_checkpoints = true;
int sync_method = DEFAULT_SYNC_METHOD;
-int wal_level = WAL_LEVEL_MINIMAL;
+int wal_level = WAL_LEVEL_REPLICA;
int CommitDelay = 0; /* precommit delay in microseconds */
int CommitSiblings = 5; /* # concurrent xacts needed to sleep */
int wal_retrieve_retry_interval = 5000;
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index e3e54713e8..c8963572a7 100644
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -58,8 +58,8 @@ char *SSLECDHCurve;
/* GUC variable: if false, prefer client ciphers */
bool SSLPreferServerCiphers;
-int ssl_min_protocol_version;
-int ssl_max_protocol_version;
+int ssl_min_protocol_version = PG_TLS1_2_VERSION;
+int ssl_max_protocol_version = PG_TLS_ANY;
/* ------------------------------------------------------------ */
/* Procedures common to all secure sessions */
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 30fb576ac3..0b637ba6a2 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -196,7 +196,7 @@ BackgroundWorker *MyBgworkerEntry = NULL;
/* The socket number we are listening for connections on */
-int PostPortNumber;
+int PostPortNumber = DEF_PGPORT;
/* The directory names for Unix socket(s) */
char *Unix_socket_directories;
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 6b95381481..73d30bf619 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -142,22 +142,22 @@ bool track_io_timing = false;
* for buffers not belonging to tablespaces that have their
* effective_io_concurrency parameter set.
*/
-int effective_io_concurrency = 0;
+int effective_io_concurrency = DEFAULT_EFFECTIVE_IO_CONCURRENCY;
/*
* Like effective_io_concurrency, but used by maintenance code paths that might
* benefit from a higher setting because they work on behalf of many sessions.
* Overridden by the tablespace setting of the same name.
*/
-int maintenance_io_concurrency = 0;
+int maintenance_io_concurrency = DEFAULT_MAINTENANCE_IO_CONCURRENCY;
/*
* GUC variables about triggering kernel writeback for buffers written; OS
* dependent defaults are set via the GUC mechanism.
*/
-int checkpoint_flush_after = 0;
-int bgwriter_flush_after = 0;
-int backend_flush_after = 0;
+int checkpoint_flush_after = DEFAULT_CHECKPOINT_FLUSH_AFTER;
+int bgwriter_flush_after = DEFAULT_BGWRITER_FLUSH_AFTER;
+int backend_flush_after = DEFAULT_BACKEND_FLUSH_AFTER;
/* local state for StartBufferIO and related functions */
static BufferDesc *InProgressBuf = NULL;
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index e1b90c5de4..6ddd46a4e7 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -109,7 +109,7 @@ const struct config_enum_entry dynamic_shared_memory_options[] = {
};
/* Implementation selector. */
-int dynamic_shared_memory_type;
+int dynamic_shared_memory_type = DEFAULT_DYNAMIC_SHARED_MEMORY_TYPE;
/* Amount of space reserved for DSM segments in the main area. */
int min_dynamic_shared_memory;
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index d32cb11436..8cd5262a3c 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -94,8 +94,8 @@
/* GUC variables */
-int xmlbinary;
-int xmloption;
+int xmlbinary = XMLBINARY_BASE64;
+int xmloption = XMLOPTION_CONTENT;
#ifdef USE_LIBXML
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 0d6a295674..cc943205d3 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -116,7 +116,7 @@ static void PlanCacheObjectCallback(Datum arg, int cacheid, uint32 hashvalue);
static void PlanCacheSysCallback(Datum arg, int cacheid, uint32 hashvalue);
/* GUC parameter */
-int plan_cache_mode;
+int plan_cache_mode = PLAN_CACHE_MODE_AUTO;
/*
* InitPlanCache: initialize module during InitPostgres.
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 6e0a66c29e..2585e24845 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -107,7 +107,7 @@ extern bool redirection_done;
emit_log_hook_type emit_log_hook = NULL;
/* GUC parameters */
-int Log_error_verbosity = PGERROR_VERBOSE;
+int Log_error_verbosity = PGERROR_DEFAULT;
char *Log_line_prefix = NULL; /* format for extra log line info */
int Log_destination = LOG_DESTINATION_STDERR;
char *Log_destination_string = NULL;
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 1a5d29ac9b..00bceec8fa 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -133,8 +133,8 @@ int max_parallel_maintenance_workers = 2;
* MaxBackends is computed by PostmasterMain after modules have had a chance to
* register background workers.
*/
-int NBuffers = 1000;
-int MaxConnections = 90;
+int NBuffers = 16384;
+int MaxConnections = 100;
int max_worker_processes = 8;
int max_parallel_workers = 8;
int MaxBackends = 0;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 6f21752b84..6657f0d92e 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1382,6 +1382,89 @@ check_GUC_name_for_parameter_acl(const char *name)
return false;
}
+/*
+ * Routine in charge of checking that the initial value of a GUC is the
+ * same when declared and when loaded to prevent anybody looking at the
+ * C declarations of these GUCS from being fooled by mismatched values.
+ *
+ * The following validation rules apply:
+ * bool - can be false, otherwise must be same as the boot_val
+ * int - can be 0, otherwise must be same as the boot_val
+ * real - can be 0.0, otherwise must be same as the boot_val
+ * string - can be NULL, otherwise must be strcmp equal to the boot_val
+ * enum - must be same as the boot_val
+ */
+#ifdef USE_ASSERT_CHECKING
+static bool
+check_GUC_init(struct config_generic *gconf)
+{
+ switch (gconf->vartype)
+ {
+ case PGC_BOOL:
+ {
+ struct config_bool *conf = (struct config_bool *) gconf;
+
+ if (*conf->variable && !conf->boot_val)
+ {
+ elog(LOG, "GUC (PGC_BOOL) %s, boot_val=%d, C-var=%d",
+ conf->gen.name, conf->boot_val, *conf->variable);
+ return false;
+ }
+ break;
+ }
+ case PGC_INT:
+ {
+ struct config_int *conf = (struct config_int *) gconf;
+
+ if (*conf->variable != 0 && *conf->variable != conf->boot_val)
+ {
+ elog(LOG, "GUC (PGC_INT) %s, boot_val=%d, C-var=%d",
+ conf->gen.name, conf->boot_val, *conf->variable);
+ return false;
+ }
+ break;
+ }
+ case PGC_REAL:
+ {
+ struct config_real *conf = (struct config_real *) gconf;
+
+ if (*conf->variable != 0.0 && *conf->variable != conf->boot_val)
+ {
+ elog(LOG, "GUC (PGC_REAL) %s, boot_val=%g, C-var=%g",
+ conf->gen.name, conf->boot_val, *conf->variable);
+ return false;
+ }
+ break;
+ }
+ case PGC_STRING:
+ {
+ struct config_string *conf = (struct config_string *) gconf;
+
+ if (*conf->variable != NULL && strcmp(*conf->variable, conf->boot_val) != 0)
+ {
+ elog(LOG, "GUC (PGC_STRING) %s, boot_val=%s, C-var=%s",
+ conf->gen.name, conf->boot_val ? conf->boot_val : "<null>", *conf->variable);
+ return false;
+ }
+ break;
+ }
+ case PGC_ENUM:
+ {
+ struct config_enum *conf = (struct config_enum *) gconf;
+
+ if (*conf->variable != conf->boot_val)
+ {
+ elog(LOG, "GUC (PGC_ENUM) %s, boot_val=%d, C-var=%d",
+ conf->gen.name, conf->boot_val, *conf->variable);
+ return false;
+ }
+ break;
+ }
+ }
+
+ return true;
+}
+#endif
/*
* Initialize GUC options during program startup.
@@ -1413,6 +1496,9 @@ InitializeGUCOptions(void)
hash_seq_init(&status, guc_hashtab);
while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL)
{
+ /* check mapping between initial and default value */
+ Assert(check_GUC_init(hentry->gucvar));
+
InitializeOneGUCOption(hentry->gucvar);
}
@@ -4654,6 +4740,9 @@ define_custom_variable(struct config_generic *variable)
GUCHashEntry *hentry;
struct config_string *pHolder;
+ /* check mapping between initial and default value */
+ Assert(check_GUC_init(variable));
+
/*
* See if there's a placeholder by the same name.
*/
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 05ab087934..836b49484a 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -526,7 +526,7 @@ int ssl_renegotiation_limit;
* This really belongs in pg_shmem.c, but is defined here so that it doesn't
* need to be duplicated in all the different implementations of pg_shmem.c.
*/
-int huge_pages;
+int huge_pages = HUGE_PAGES_TRY;
int huge_page_size;
/*
@@ -543,7 +543,14 @@ static char *locale_ctype;
static char *server_encoding_string;
static char *server_version_string;
static int server_version_num;
-static int syslog_facility;
+
+#ifdef HAVE_SYSLOG
+#define DEFAULT_SYSLOG_FACILITY LOG_LOCAL0
+#else
+#define DEFAULT_SYSLOG_FACILITY 0
+#endif
+static int syslog_facility = DEFAULT_SYSLOG_FACILITY;
+
static char *timezone_string;
static char *log_timezone_string;
static char *timezone_abbreviations_string;
@@ -559,7 +566,14 @@ static int shared_memory_size_in_huge_pages;
static int wal_block_size;
static bool data_checksums;
static bool integer_datetimes;
-static bool assert_enabled;
+
+#ifdef USE_ASSERT_CHECKING
+#define DEFAULT_ASSERT_ENABLED true
+#else
+#define DEFAULT_ASSERT_ENABLED false
+#endif
+static bool assert_enabled = DEFAULT_ASSERT_ENABLED;
+
static char *recovery_target_timeline_string;
static char *recovery_target_string;
static char *recovery_target_xid_string;
@@ -1181,11 +1195,7 @@ struct config_bool ConfigureNamesBool[] =
GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
},
&assert_enabled,
-#ifdef USE_ASSERT_CHECKING
- true,
-#else
- false,
-#endif
+ DEFAULT_ASSERT_ENABLED,
NULL, NULL, NULL
},
@@ -1357,11 +1367,7 @@ struct config_bool ConfigureNamesBool[] =
gettext_noop("Enables updating of the process title every time a new SQL command is received by the server.")
},
&update_process_title,
-#ifdef WIN32
- false,
-#else
- true,
-#endif
+ DEFAULT_UPDATE_PROCESS_TITLE,
NULL, NULL, NULL
},
@@ -2888,11 +2894,7 @@ struct config_int ConfigureNamesInt[] =
GUC_EXPLAIN
},
&effective_io_concurrency,
-#ifdef USE_PREFETCH
- 1,
-#else
- 0,
-#endif
+ DEFAULT_EFFECTIVE_IO_CONCURRENCY,
0, MAX_IO_CONCURRENCY,
check_effective_io_concurrency, NULL, NULL
},
@@ -2906,11 +2908,7 @@ struct config_int ConfigureNamesInt[] =
GUC_EXPLAIN
},
&maintenance_io_concurrency,
-#ifdef USE_PREFETCH
- 10,
-#else
- 0,
-#endif
+ DEFAULT_MAINTENANCE_IO_CONCURRENCY,
0, MAX_IO_CONCURRENCY,
check_maintenance_io_concurrency, assign_maintenance_io_concurrency,
NULL
@@ -4613,11 +4611,7 @@ struct config_enum ConfigureNamesEnum[] =
NULL
},
&syslog_facility,
-#ifdef HAVE_SYSLOG
- LOG_LOCAL0,
-#else
- 0,
-#endif
+ DEFAULT_SYSLOG_FACILITY,
syslog_facility_options,
NULL, assign_syslog_facility, NULL
},
diff --git a/src/backend/utils/misc/ps_status.c b/src/backend/utils/misc/ps_status.c
index 8520ce76bb..d81a67be79 100644
--- a/src/backend/utils/misc/ps_status.c
+++ b/src/backend/utils/misc/ps_status.c
@@ -30,8 +30,9 @@
#include "utils/ps_status.h"
extern char **environ;
-bool update_process_title = true;
+/* GUC variable */
+bool update_process_title = DEFAULT_UPDATE_PROCESS_TITLE;
/*
* Alternative ways of updating ps display:
diff --git a/contrib/auth_delay/auth_delay.c b/contrib/auth_delay/auth_delay.c
index c3d78e5020..4ca9b4afb1 100644
--- a/contrib/auth_delay/auth_delay.c
+++ b/contrib/auth_delay/auth_delay.c
@@ -21,7 +21,7 @@
PG_MODULE_MAGIC;
/* GUC Variables */
-static int auth_delay_milliseconds;
+static int auth_delay_milliseconds = 0;
/* Original Hook */
static ClientAuthentication_hook_type original_client_auth_hook = NULL;
diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index 1843b1862e..fe002b17e0 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -103,7 +103,7 @@ static AutoPrewarmSharedState *apw_state = NULL;
/* GUC variables. */
static bool autoprewarm = true; /* start worker? */
-static int autoprewarm_interval; /* dump interval */
+static int autoprewarm_interval = 300; /* dump interval */
/*
* Module load callback.
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index e5aa429995..198e4e84ad 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -283,11 +283,11 @@ static const struct config_enum_entry track_options[] =
{NULL, 0, false}
};
-static int pgss_max; /* max # statements to track */
-static int pgss_track; /* tracking level */
-static bool pgss_track_utility; /* whether to track utility commands */
-static bool pgss_track_planning; /* whether to track planning duration */
-static bool pgss_save; /* whether to save stats across shutdown */
+static int pgss_max = 5000; /* max # statements to track */
+static int pgss_track = PGSS_TRACK_TOP; /* tracking level */
+static bool pgss_track_utility = true; /* whether to track utility commands */
+static bool pgss_track_planning = false ; /* whether to track planning duration */
+static bool pgss_save = true; /* whether to save stats across shutdown */
#define pgss_enabled(level) \
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index 154346398a..2c644bc148 100644
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -68,7 +68,7 @@ _PG_init(void)
"Sets the threshold used by the % operator.",
"Valid range is 0.0 .. 1.0.",
&similarity_threshold,
- 0.3,
+ 0.3f,
0.0,
1.0,
PGC_USERSET,
@@ -80,7 +80,7 @@ _PG_init(void)
"Sets the threshold used by the <% operator.",
"Valid range is 0.0 .. 1.0.",
&word_similarity_threshold,
- 0.6,
+ 0.6f,
0.0,
1.0,
PGC_USERSET,
@@ -92,7 +92,7 @@ _PG_init(void)
"Sets the threshold used by the <<% operator.",
"Valid range is 0.0 .. 1.0.",
&strict_word_similarity_threshold,
- 0.5,
+ 0.5f,
0.0,
1.0,
PGC_USERSET,
diff --git a/contrib/sepgsql/hooks.c b/contrib/sepgsql/hooks.c
index 87fdd972c2..363ac06700 100644
--- a/contrib/sepgsql/hooks.c
+++ b/contrib/sepgsql/hooks.c
@@ -57,7 +57,7 @@ static sepgsql_context_info_t sepgsql_context_info;
/*
* GUC: sepgsql.permissive = (on|off)
*/
-static bool sepgsql_permissive;
+static bool sepgsql_permissive = false;
bool
sepgsql_get_permissive(void)
@@ -68,7 +68,7 @@ sepgsql_get_permissive(void)
/*
* GUC: sepgsql.debug_audit = (on|off)
*/
-static bool sepgsql_debug_audit;
+static bool sepgsql_debug_audit = false;
bool
sepgsql_get_debug_audit(void)
--
2.37.2
signature.asc
Description: PGP signature
