From 7ab5b4eb483478bc85ad45ef5405b4a70c3f4c94 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 8 Jun 2022 13:26:18 -0400 Subject: [PATCH] Be more careful about GucSource for internally-driven GUC settings. The original advice for hard-wired SetConfigOption calls was to use PGC_S_OVERRIDE, particularly for PGC_INTERNAL GUCs. However, that's really overkill for PGC_INTERNAL GUCs, since there is no possibility that we need to override a user-provided setting. Instead use PGC_S_DYNAMIC_DEFAULT in most places, so that the value will appear with source = 'default' in pg_settings and thereby not be shown by psql's new \dconfig command. The one exception is that when changing in_hot_standby in a hot-standby session, we still use PGC_S_OVERRIDE, because people felt that seeing that in \dconfig would be a good thing. Similarly use PGC_S_DYNAMIC_DEFAULT for the auto-tune value of wal_buffers (if possible, that is if wal_buffers wasn't explicitly set to -1), and for the typical 2MB value of max_stack_depth. In combination these changes remove four not-very-interesting entries from the typical output of \dconfig, all of which people fingered as "why is that showing up?" in the discussion thread. Discussion: https://postgr.es/m/3118455.1649267333@sss.pgh.pa.us --- src/backend/access/transam/xlog.c | 15 ++++++++++--- src/backend/bootstrap/bootstrap.c | 2 +- src/backend/postmaster/postmaster.c | 2 +- src/backend/storage/ipc/ipci.c | 6 +++-- src/backend/utils/init/miscinit.c | 6 ++--- src/backend/utils/init/postinit.c | 6 ++--- src/backend/utils/misc/guc-file.l | 5 ++--- src/backend/utils/misc/guc.c | 34 +++++++++++++++++------------ src/include/utils/guc.h | 8 +++++-- 9 files changed, 52 insertions(+), 32 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 71136b11a2..8764084e21 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -4167,7 +4167,7 @@ ReadControlFile(void) snprintf(wal_segsz_str, sizeof(wal_segsz_str), "%d", wal_segment_size); SetConfigOption("wal_segment_size", wal_segsz_str, PGC_INTERNAL, - PGC_S_OVERRIDE); + PGC_S_DYNAMIC_DEFAULT); /* check and update variables dependent on wal_segment_size */ if (ConvertToXSegs(min_wal_size_mb, wal_segment_size) < 2) @@ -4186,7 +4186,7 @@ ReadControlFile(void) /* Make the initdb settings visible as GUC variables, too */ SetConfigOption("data_checksums", DataChecksumsEnabled() ? "yes" : "no", - PGC_INTERNAL, PGC_S_OVERRIDE); + PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT); } /* @@ -4343,13 +4343,22 @@ XLOGShmemSize(void) * This isn't an amazingly clean place to do this, but we must wait till * NBuffers has received its final value, and must do it before using the * value of XLOGbuffers to do anything important. + * + * We prefer to report this value's source as PGC_S_DYNAMIC_DEFAULT. + * However, if the DBA explicitly set wal_buffers = -1 in the config file, + * then PGC_S_DYNAMIC_DEFAULT will fail to override that and we must force + * the matter with PGC_S_OVERRIDE. */ if (XLOGbuffers == -1) { char buf[32]; snprintf(buf, sizeof(buf), "%d", XLOGChooseNumBuffers()); - SetConfigOption("wal_buffers", buf, PGC_POSTMASTER, PGC_S_OVERRIDE); + SetConfigOption("wal_buffers", buf, PGC_POSTMASTER, + PGC_S_DYNAMIC_DEFAULT); + if (XLOGbuffers == -1) /* failed to apply it? */ + SetConfigOption("wal_buffers", buf, PGC_POSTMASTER, + PGC_S_OVERRIDE); } Assert(XLOGbuffers > 0); diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 9fa8fdd4cf..9a610d41ad 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -262,7 +262,7 @@ BootstrapModeMain(int argc, char *argv[], bool check_only) (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_OVERRIDE); + PGC_S_DYNAMIC_DEFAULT); } break; case 'c': diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 3b73e26956..dde4bc25b1 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -921,7 +921,7 @@ PostmasterMain(int argc, char *argv[]) * useful to show, even if one would only expect at least PANIC. LOG * entries are hidden. */ - SetConfigOption("log_min_messages", "FATAL", PGC_INTERNAL, + SetConfigOption("log_min_messages", "FATAL", PGC_SUSET, PGC_S_OVERRIDE); } diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c index 26372d95b3..1a6f527051 100644 --- a/src/backend/storage/ipc/ipci.c +++ b/src/backend/storage/ipc/ipci.c @@ -334,7 +334,8 @@ InitializeShmemGUCs(void) size_b = CalculateShmemSize(NULL); size_mb = add_size(size_b, (1024 * 1024) - 1) / (1024 * 1024); sprintf(buf, "%zu", size_mb); - SetConfigOption("shared_memory_size", buf, PGC_INTERNAL, PGC_S_OVERRIDE); + SetConfigOption("shared_memory_size", buf, + PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT); /* * Calculate the number of huge pages required. @@ -346,6 +347,7 @@ InitializeShmemGUCs(void) hp_required = add_size(size_b / hp_size, 1); sprintf(buf, "%zu", hp_required); - SetConfigOption("shared_memory_size_in_huge_pages", buf, PGC_INTERNAL, PGC_S_OVERRIDE); + SetConfigOption("shared_memory_size_in_huge_pages", buf, + PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT); } } diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index ec6a61594a..b25bd0e583 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -787,7 +787,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid) PGC_BACKEND, PGC_S_OVERRIDE); SetConfigOption("is_superuser", AuthenticatedUserIsSuperuser ? "on" : "off", - PGC_INTERNAL, PGC_S_OVERRIDE); + PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT); ReleaseSysCache(roleTup); } @@ -844,7 +844,7 @@ SetSessionAuthorization(Oid userid, bool is_superuser) SetConfigOption("is_superuser", is_superuser ? "on" : "off", - PGC_INTERNAL, PGC_S_OVERRIDE); + PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT); } /* @@ -901,7 +901,7 @@ SetCurrentRoleId(Oid roleid, bool is_superuser) SetConfigOption("is_superuser", is_superuser ? "on" : "off", - PGC_INTERNAL, PGC_S_OVERRIDE); + PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT); } diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index fa701daa26..6b9082604f 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -391,7 +391,7 @@ CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connect SetDatabaseEncoding(dbform->encoding); /* Record it as a GUC internal option, too */ SetConfigOption("server_encoding", GetDatabaseEncodingName(), - PGC_INTERNAL, PGC_S_OVERRIDE); + PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT); /* If we have no other source of client_encoding, use server encoding */ SetConfigOption("client_encoding", GetDatabaseEncodingName(), PGC_BACKEND, PGC_S_DYNAMIC_DEFAULT); @@ -470,8 +470,8 @@ CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connect } /* Make the locale settings visible as GUC variables, too */ - SetConfigOption("lc_collate", collate, PGC_INTERNAL, PGC_S_OVERRIDE); - SetConfigOption("lc_ctype", ctype, PGC_INTERNAL, PGC_S_OVERRIDE); + SetConfigOption("lc_collate", collate, PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT); + SetConfigOption("lc_ctype", ctype, PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT); check_strxfrm_bug(); diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l index 4360c461df..ce5633844c 100644 --- a/src/backend/utils/misc/guc-file.l +++ b/src/backend/utils/misc/guc-file.l @@ -401,9 +401,8 @@ ProcessConfigFileInternal(GucContext context, bool applySettings, int elevel) * be run only after initialization is complete. * * XXX this is an unmaintainable crock, because we have to know how to set - * (or at least what to call to set) every variable that could potentially - * have PGC_S_DYNAMIC_DEFAULT or PGC_S_ENV_VAR source. However, there's no - * time to redesign it for 9.1. + * (or at least what to call to set) every non-PGC_INTERNAL variable that + * could potentially have PGC_S_DYNAMIC_DEFAULT or PGC_S_ENV_VAR source. */ if (context == PGC_SIGHUP && applySettings) { diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 55d41ae7d6..a7cc49898b 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -5939,6 +5939,8 @@ InitializeGUCOptionsFromEnvironment(void) * rlimit isn't exactly an "environment variable", but it behaves about * the same. If we can identify the platform stack depth rlimit, increase * default stack depth setting up to whatever is safe (but at most 2MB). + * Report the value's source as PGC_S_DYNAMIC_DEFAULT if it's 2MB, or as + * PGC_S_ENV_VAR if it's reflecting the rlimit limit. */ stack_rlimit = get_stack_depth_rlimit(); if (stack_rlimit > 0) @@ -5947,12 +5949,19 @@ InitializeGUCOptionsFromEnvironment(void) if (new_limit > 100) { + GucSource source; char limbuf[16]; - new_limit = Min(new_limit, 2048); - sprintf(limbuf, "%ld", new_limit); + if (new_limit < 2048) + source = PGC_S_ENV_VAR; + else + { + new_limit = 2048; + source = PGC_S_DYNAMIC_DEFAULT; + } + snprintf(limbuf, sizeof(limbuf), "%ld", new_limit); SetConfigOption("max_stack_depth", limbuf, - PGC_POSTMASTER, PGC_S_ENV_VAR); + PGC_POSTMASTER, source); } } } @@ -6776,12 +6785,16 @@ BeginReportingGUCOptions(void) reporting_enabled = true; /* - * Hack for in_hot_standby: initialize with the value we're about to send. + * Hack for in_hot_standby: set the GUC value true if appropriate. This + * is kind of an ugly place to do it, but there's few better options. + * * (This could be out of date by the time we actually send it, in which * case the next ReportChangedGUCOptions call will send a duplicate * report.) */ - in_hot_standby = RecoveryInProgress(); + if (RecoveryInProgress()) + SetConfigOption("in_hot_standby", "true", + PGC_INTERNAL, PGC_S_OVERRIDE); /* Transmit initial values of interesting variables */ for (i = 0; i < num_guc_variables; i++) @@ -6822,15 +6835,8 @@ ReportChangedGUCOptions(void) * transition from false to true. */ if (in_hot_standby && !RecoveryInProgress()) - { - struct config_generic *record; - - record = find_option("in_hot_standby", false, false, ERROR); - Assert(record != NULL); - record->status |= GUC_NEEDS_REPORT; - report_needed = true; - in_hot_standby = false; - } + SetConfigOption("in_hot_standby", "false", + PGC_INTERNAL, PGC_S_OVERRIDE); /* Quick exit if no values have been changed */ if (!report_needed) diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index efcbad7842..d5976ecbfb 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -83,8 +83,7 @@ typedef enum * override the postmaster command line.) Tracking the source allows us * to process sources in any convenient order without affecting results. * Sources <= PGC_S_OVERRIDE will set the default used by RESET, as well - * as the current value. Note that source == PGC_S_OVERRIDE should be - * used when setting a PGC_INTERNAL option. + * as the current value. * * PGC_S_INTERACTIVE isn't actually a source value, but is the * dividing line between "interactive" and "non-interactive" sources for @@ -99,6 +98,11 @@ typedef enum * shouldn't throw hard errors in this case, at most NOTICEs, since the * objects might exist by the time the setting is used for real. * + * When setting the value of a non-compile-time-constant PGC_INTERNAL option, + * source == PGC_S_DYNAMIC_DEFAULT should typically be used so that the value + * will show as "default" in pg_settings. If there is a specific reason not + * to want that, use source == PGC_S_OVERRIDE. + * * NB: see GucSource_Names in guc.c if you change this. */ typedef enum