Re: How about a psql backslash command to show GUCs? - Mailing list pgsql-hackers

Christoph Berg <myon@debian.org> writes:
> in_hot_standby sounds very useful to have in that list.

I thought about this some more and concluded that we're blaming the
messenger.  There's nothing wrong with \dconfig's filtering rule;
rather, it's the fault of the backend for mislabeling a lot of
run-time-computed values as source=PGC_S_OVERRIDE when they really
are dynamic defaults.  Now in fairness, I think a lot of that code
predates the invention of PGC_S_DYNAMIC_DEFAULT, so there was not
a better way when it was written ... but there is now.

The attached draft patch makes the following changes:

* All run-time calculations of PGC_INTERNAL variables are relabeled
as PGC_S_DYNAMIC_DEFAULT.  There is not any higher source value
that we'd allow to set such a variable, so this is sufficient.
(I didn't do anything about in_hot_standby, which is set through
a hack rather than via set_config_option; not sure whether we want
to do anything there, or what it should be if we do.)  The net
effect of this compared to upthread examples is to hide
shared_memory_size and shared_memory_size_in_huge_pages from \dconfig.

* The auto-tune value of wal_buffers is relabeled as PGC_S_DYNAMIC_DEFAULT
if possible (but due to pre-existing hacks, we might have to force it).
This will hide that one too as long as you didn't manually set it.

* The rlimit-derived value of max_stack_depth is likewise relabeled
as PGC_S_DYNAMIC_DEFAULT, resolving the complaint Jonathan had upthread.
But now that we have a way to hide this, I'm having second thoughts
about whether we should.  If you are on a platform that's forcing an
unreasonably small stack size, it'd be good if \dconfig told you so.
Could it be sane to label that value as PGC_S_DYNAMIC_DEFAULT only when
it's the limit value (2MB) and PGC_S_ENV_VAR when it's smaller?

In any case, I expect that we'd apply this patch only to HEAD, which
means that when using psql's \dconfig against a pre-v15 server,
you'd still see these settings that we're trying to hide.
That doesn't bother me too much, but maybe some would find it
confusing.

Thoughts?

            regards, tom lane

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 71136b11a2..2461317ee4 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 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..2e21bf0338 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).
+     * (But since this isn't really an environment variable, report the
+     * setting as PGC_S_DYNAMIC_DEFAULT not PGC_S_ENV_VAR.)
      */
     stack_rlimit = get_stack_depth_rlimit();
     if (stack_rlimit > 0)
@@ -5952,7 +5954,7 @@ InitializeGUCOptionsFromEnvironment(void)
             new_limit = Min(new_limit, 2048);
             sprintf(limbuf, "%ld", new_limit);
             SetConfigOption("max_stack_depth", limbuf,
-                            PGC_POSTMASTER, PGC_S_ENV_VAR);
+                            PGC_POSTMASTER, PGC_S_DYNAMIC_DEFAULT);
         }
     }
 }
@@ -7618,6 +7620,8 @@ set_config_option(const char *name, const char *value,
                                 name)));
                 return 0;
             }
+            /* The source should be a boot_val or a dynamic default. */
+            Assert(source <= PGC_S_DYNAMIC_DEFAULT);
             break;
         case PGC_POSTMASTER:
             if (context == PGC_SIGHUP)
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index efcbad7842..385c97fc76 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -83,8 +83,8 @@ 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.  Note that source == PGC_S_DYNAMIC_DEFAULT should
+ * be used when setting a non-compile-time-constant PGC_INTERNAL option.
  *
  * PGC_S_INTERACTIVE isn't actually a source value, but is the
  * dividing line between "interactive" and "non-interactive" sources for