Be more careful about GucSource for internally-driven GUC settings.
authorTom Lane <tgl@sss.pgh.pa.us>
Wed, 8 Jun 2022 17:26:18 +0000 (13:26 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Wed, 8 Jun 2022 17:26:18 +0000 (13:26 -0400)
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
src/backend/bootstrap/bootstrap.c
src/backend/postmaster/postmaster.c
src/backend/storage/ipc/ipci.c
src/backend/utils/init/miscinit.c
src/backend/utils/init/postinit.c
src/backend/utils/misc/guc-file.l
src/backend/utils/misc/guc.c
src/include/utils/guc.h

index 71136b11a2a0936d5132ab114fd775b8550206a0..8764084e215933a29f6563df600c87f1e6966dbb 100644 (file)
@@ -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);
 
index 9fa8fdd4cf3c265790a141a8028208bc64e89d27..9a610d41ad7d5038a79dc3adb3d9c7927cf0c335 100644 (file)
@@ -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':
index 3b73e26956473cf872bdc700510f410d72e45749..dde4bc25b1360bdfdeb945c59bb72f64d288dc5d 100644 (file)
@@ -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);
        }
 
index 26372d95b3856c6014f24dcfe448461563ec5729..1a6f527051845ff2dac3d46a987ff68943cb3e5e 100644 (file)
@@ -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);
        }
 }
index ec6a61594a4b27f12896eeb05d0f49136b992499..b25bd0e583850add127589715dfa2f2cccb71674 100644 (file)
@@ -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);
 }
 
 
index fa701daa26f9326329adda3d36ada3fe3ac628e2..6b9082604fb7213570ef8ab8a389db69ea0c3de7 100644 (file)
@@ -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();
 
index 4360c461dfedf11644c7c8da26e7b4ceb7bafaf6..ce5633844c343230ef509c24f318591f4fa4f604 100644 (file)
@@ -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)
        {
index 55d41ae7d63718ceb1c4602cdf12670eedfe525f..a7cc49898b0289d9e76aeec05e1e7fd4085c2745 100644 (file)
@@ -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)
index efcbad78423e96d11a1e2755e82ec96658289a86..d5976ecbfb9d112b9db7663df84394886a6cbdf1 100644 (file)
@@ -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