Code review for GUC revert-values-if-removed-from-postgresql.conf patch;
authorTom Lane
Mon, 10 Sep 2007 00:57:22 +0000 (00:57 +0000)
committerTom Lane
Mon, 10 Sep 2007 00:57:22 +0000 (00:57 +0000)
and in passing, fix some bogosities dating from the custom_variable_classes
patch.  Fix guc-file.l to correctly check changes in custom_variable_classes
that are attempted concurrently with additions/removals of custom variables,
and don't allow the new setting to be applied in advance of checking it.
Clean up messy and undocumented situation for string variables with NULL
boot_val.  Fix DefineCustomVariable functions to initialize boot_val
correctly.  Prevent find_option from inserting bogus placeholders for custom
variables that are simply inquired about rather than being set.

src/backend/utils/cache/ts_cache.c
src/backend/utils/misc/README
src/backend/utils/misc/guc-file.l
src/backend/utils/misc/guc.c
src/include/utils/guc_tables.h

index 94051b8c32f1233c3c9e259bb349bb617b31c951..63c57905565a25615357b79daa245c8a315eff87 100644 (file)
@@ -20,7 +20,7 @@
  * Copyright (c) 2006-2007, PostgreSQL Global Development Group
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/utils/cache/ts_cache.c,v 1.2 2007/08/22 01:39:45 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/utils/cache/ts_cache.c,v 1.3 2007/09/10 00:57:21 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -592,14 +592,6 @@ getTSCurrentConfig(bool emitError)
 const char *
 assignTSCurrentConfig(const char *newval, bool doit, GucSource source)
 {
-   /* do nothing during initial GUC setup */
-   if (newval == NULL)
-   {
-       if (doit)
-           TSCurrentConfigCache = InvalidOid;
-       return newval;
-   }
-
    /*
     * If we aren't inside a transaction, we cannot do database access so
     * cannot verify the config name.  Must accept it on faith.
@@ -637,7 +629,7 @@ assignTSCurrentConfig(const char *newval, bool doit, GucSource source)
        newval = strdup(buf);
        pfree(buf);
 
-       if (doit)
+       if (doit && newval)
            TSCurrentConfigCache = cfgId;
    }
    else
index 3ea838b1f5312de3ed6ca79fa79274f48d871540..bf1e3b654555452ecb565fb26dc169d87e694cda 100644 (file)
@@ -1,4 +1,4 @@
-$PostgreSQL: pgsql/src/backend/utils/misc/README,v 1.5 2004/07/01 00:51:24 tgl Exp $
+$PostgreSQL: pgsql/src/backend/utils/misc/README,v 1.6 2007/09/10 00:57:21 tgl Exp $
 
 
 GUC IMPLEMENTATION NOTES
@@ -53,6 +53,14 @@ The third choice is allowed in case the assign_hook wants to return a
 datestyle always returns a string that includes both output and input
 datestyle options, although the input might have specified only one.
 
+Note that a string variable's assign_hook will NEVER be called with a NULL
+value for newvalue, since there would be no way to distinguish success
+and failure returns.  If the boot_val or reset_val for a string variable
+is NULL, it will just be assigned without calling the assign_hook.
+Therefore, a NULL boot_val should never be used in combination with an
+assign_hook that has side-effects, as the side-effects wouldn't happen
+during a RESET that re-institutes the boot-time setting.
+
 If a show_hook is provided, it points to a function of the signature
    const char *show_hook(void)
 This hook allows variable-specific computation of the value displayed
index 4327601320ed02b454bfe326728bb515524b2578..124dfbf62e1570d23a584819e8b84f849308a6cd 100644 (file)
@@ -4,7 +4,7 @@
  *
  * Copyright (c) 2000-2007, PostgreSQL Global Development Group
  *
- * $PostgreSQL: pgsql/src/backend/utils/misc/guc-file.l,v 1.50 2007/04/21 20:02:40 petere Exp $
+ * $PostgreSQL: pgsql/src/backend/utils/misc/guc-file.l,v 1.51 2007/09/10 00:57:21 tgl Exp $
  */
 
 %{
@@ -116,9 +116,10 @@ ProcessConfigFile(GucContext context)
 {
    int         elevel;
    struct name_value_pair *item, *head, *tail;
+   char       *cvc = NULL;
+   struct config_string *cvc_struct;
+   const char *envvar;
    int         i;
-   bool       *in_conffile = NULL;
-   const char *var;
 
    Assert(context == PGC_POSTMASTER || context == PGC_SIGHUP);
 
@@ -133,6 +134,7 @@ ProcessConfigFile(GucContext context)
    else
        elevel = ERROR;
 
+   /* Parse the file into a list of option names and values */
    head = tail = NULL;
 
    if (!ParseConfigFile(ConfigFileName, NULL,
@@ -140,162 +142,178 @@ ProcessConfigFile(GucContext context)
                         &head, &tail))
        goto cleanup_list;
 
-   /* Check if all options are valid */
-   for (item = head; item; item = item->next)
+   /*
+    * We need the proposed new value of custom_variable_classes to check
+    * custom variables with.  ParseConfigFile ensured that if it's in
+    * the file, it's first in the list.  But first check to see if we
+    * have an active value from the command line, which should override
+    * the file in any case.  (Since there's no relevant env var, the
+    * only possible nondefault sources are the file and ARGV.)
+    */
+   cvc_struct = (struct config_string *)
+       find_option("custom_variable_classes", false, elevel);
+   if (cvc_struct && cvc_struct->gen.reset_source > PGC_S_FILE)
    {
-       char *sep = strchr(item->name, GUC_QUALIFIER_SEPARATOR);
-       if (sep && !is_custom_class(item->name, sep - item->name))
+       cvc = guc_strdup(elevel, cvc_struct->reset_val);
+       if (cvc == NULL)
+           goto cleanup_list;
+   }
+   else if (head != NULL &&
+            guc_name_compare(head->name, "custom_variable_classes") == 0)
+   {
+       /*
+        * Need to canonicalize the value via the assign hook.  Casting away
+        * const is a bit ugly, but we know the result is malloc'd.
+        */
+       cvc = (char *) assign_custom_variable_classes(head->value,
+                                                     false, PGC_S_FILE);
+       if (cvc == NULL)
        {
            ereport(elevel,
-               (errcode(ERRCODE_UNDEFINED_OBJECT),
-                errmsg("unrecognized configuration parameter \"%s\"",
-                       item->name)));
+                   (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                    errmsg("invalid value for parameter \"%s\": \"%s\"",
+                           head->name, head->value)));
            goto cleanup_list;
        }
-
-       if (!set_config_option(item->name, item->value, context,
-                              PGC_S_FILE, false, false))
-           goto cleanup_list;
    }
 
-
    /*
-    * Mark all variables as not showing up in the config file.  The
-    * allocation has to take place after ParseConfigFile() since this
-    * function can change num_guc_variables due to custom variables.
-    * It would be easier to add a new field or status bit to struct
-    * conf_generic, but that way we would expose internal information
-    * that is just needed here in the following few lines.  The price
-    * to pay for this separation are a few more loops over the set of
-    * configuration options, but those are expected to be rather few
-    * and we only have to pay the cost at SIGHUP.  We initialize
-    * in_conffile only here because set_config_option() makes
-    * guc_variables grow with custom variables.
+    * Mark all extant GUC variables as not present in the config file.
+    * We need this so that we can tell below which ones have been removed
+    * from the file since we last processed it.
     */
-   in_conffile = guc_malloc(elevel, num_guc_variables * sizeof(bool));
-   if (!in_conffile)
-       goto cleanup_list;
    for (i = 0; i < num_guc_variables; i++)
-       in_conffile[i] = false;
+   {
+       struct config_generic *gconf = guc_variables[i];
+
+       gconf->status &= ~GUC_IS_IN_FILE;
+   }
 
+   /*
+    * Check if all options are valid.  As a side-effect, the GUC_IS_IN_FILE
+    * flag is set on each GUC variable mentioned in the list.
+    */
    for (item = head; item; item = item->next)
    {
-       /*
-        * After set_config_option() the variable name item->name is
-        * known to exist.
-        */
-       Assert(guc_get_index(item->name) >= 0);
-       in_conffile[guc_get_index(item->name)] = true;
+       char *sep = strchr(item->name, GUC_QUALIFIER_SEPARATOR);
+
+       if (sep)
+       {
+           /*
+            * We have to consider three cases for custom variables:
+            *
+            * 1. The class name is not valid according to the (new) setting
+            * of custom_variable_classes.  If so, reject.  We don't care
+            * which side is at fault.
+            */
+           if (!is_custom_class(item->name, sep - item->name, cvc))
+           {
+               ereport(elevel,
+                       (errcode(ERRCODE_UNDEFINED_OBJECT),
+                        errmsg("unrecognized configuration parameter \"%s\"",
+                               item->name)));
+               goto cleanup_list;
+           }
+           /*
+            * 2. There is no GUC entry.  If we called set_config_option then
+            * it would make a placeholder, which we don't want to do yet,
+            * since we could still fail further down the list.  Do nothing
+            * (assuming that making the placeholder will succeed later).
+            */
+           if (find_option(item->name, false, elevel) == NULL)
+               continue;
+           /*
+            * 3. There is already a GUC entry (either real or placeholder) for
+            * the variable.  In this case we should let set_config_option
+            * check it, since the assignment could well fail if it's a real
+            * entry.
+            */
+       }
+
+       if (!set_config_option(item->name, item->value, context,
+                              PGC_S_FILE, false, false))
+           goto cleanup_list;
    }
 
+   /*
+    * Check for variables having been removed from the config file, and
+    * revert their reset values (and perhaps also effective values) to the
+    * boot-time defaults.  If such a variable can't be changed after startup,
+    * just throw a warning and continue.  (This is analogous to the fact that
+    * set_config_option only throws a warning for a new but different value.
+    * If we wanted to make it a hard error, we'd need an extra pass over the
+    * list so that we could throw the error before starting to apply
+    * changes.)
+    */
    for (i = 0; i < num_guc_variables; i++)
    {
        struct config_generic *gconf = guc_variables[i];
-       if (!in_conffile[i] && gconf->source == PGC_S_FILE)
+       GucStack   *stack;
+
+       if (gconf->reset_source != PGC_S_FILE ||
+           (gconf->status & GUC_IS_IN_FILE))
+           continue;
+       if (gconf->context < PGC_SIGHUP)
        {
-           if (gconf->context < PGC_SIGHUP)
-               ereport(elevel,
+           ereport(elevel,
                    (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
-                            errmsg("parameter \"%s\" cannot be changed after server start; configuration file change ignored",
-                                   gconf->name)));
-           else
-           {
-                   /* prepare */
-                   GucStack   *stack;
-                   if (gconf->reset_source == PGC_S_FILE)
-                       gconf->reset_source = PGC_S_DEFAULT;
-                   for (stack = gconf->stack; stack; stack = stack->prev)
-                       if (stack->source == PGC_S_FILE)
-                           stack->source = PGC_S_DEFAULT;
-                   /* apply the default */
-                   set_config_option(gconf->name, NULL, context,
-                                     PGC_S_DEFAULT, false, true);
-           }
+                    errmsg("parameter \"%s\" cannot be changed after server start; configuration file change ignored",
+                           gconf->name)));
+           continue;
        }
-       else if (!in_conffile[i] && gconf->reset_source == PGC_S_FILE)
-       {
-           /*------
-            * Change the reset_val to default_val.  Here's an
-            * example: In the configuration file we have
-            *
-            * seq_page_cost = 3.00
-            *
-            * Now we execute in a session
-            *
-            * SET seq_page_cost TO 4.00;
-            *
-            * Then we remove this option from the configuration file
-            * and send SIGHUP. Now when you execute
-            *
-            * RESET seq_page_cost;
-            *
-            * it should fall back to 1.00 (the default value for
-            * seq_page_cost) and not to 3.00 (which is the current
-            * reset_val).
-            */
 
-           switch (gconf->vartype)
-           {
-               case PGC_BOOL:
-                   {
-                       struct config_bool *conf;
-                       conf = (struct config_bool *) gconf;
-                       conf->reset_val = conf->boot_val;
-                       break;
-                   }
-               case PGC_INT:
-                   {
-                       struct config_int *conf;
-                       conf = (struct config_int *) gconf;
-                       conf->reset_val = conf->boot_val;
-                       break;
-                   }
-               case PGC_REAL:
-                   {
-                       struct config_real *conf;
-                       conf = (struct config_real *) gconf;
-                       conf->reset_val = conf->boot_val;
-                       break;
-                   }
-               case PGC_STRING:
-                   {
-                       struct config_string   *conf;
-                       conf = (struct config_string *) gconf;
-                       /*
-                        * We can cast away the const here because we
-                        * won't free the address.  It is protected by
-                        * set_string_field() and string_field_used().
-                        */
-                       conf->reset_val = (char *) conf->boot_val;
-                       break;
-                   }
-           }
+       /*
+        * Reset any "file" sources to "default", else set_config_option
+        * will not override those settings.  tentative_source should
+        * never be "file".
+        */
+       if (gconf->reset_source == PGC_S_FILE)
+           gconf->reset_source = PGC_S_DEFAULT;
+       Assert(gconf->tentative_source != PGC_S_FILE);
+       if (gconf->source == PGC_S_FILE)
+           gconf->source = PGC_S_DEFAULT;
+       for (stack = gconf->stack; stack; stack = stack->prev)
+       {
+           Assert(stack->tentative_source != PGC_S_FILE);
+           if (stack->source == PGC_S_FILE)
+               stack->source = PGC_S_DEFAULT;
        }
-   }
 
-   /* If we got here all the options checked out okay, so apply them. */
-   for (item = head; item; item = item->next)
-       set_config_option(item->name, item->value, context,
-                         PGC_S_FILE, false, true);
+       /* Now we can re-apply the wired-in default */
+       set_config_option(gconf->name, NULL, context, PGC_S_DEFAULT,
+                         false, true);
+   }
 
    /*
-    * Reset variables to the value of environment variables
-    * (PGC_S_ENV_VAR overrules PGC_S_FILE).  PGPORT is ignored,
-    * because it cannot be changed without restart.
+    * Restore any variables determined by environment variables.  This
+    * is a no-op except in the case where one of these had been in the
+    * config file and is now removed.  PGC_S_ENV_VAR will override the
+    * wired-in default we just applied, but cannot override any other source.
+    * PGPORT can be ignored, because it cannot be changed without restart.
+    * Keep this list in sync with InitializeGUCOptions()!
     */
-   var = getenv("PGDATESTYLE");
-   if (var != NULL)
-       set_config_option("datestyle", var, context,
+   envvar = getenv("PGDATESTYLE");
+   if (envvar != NULL)
+       set_config_option("datestyle", envvar, PGC_POSTMASTER,
                          PGC_S_ENV_VAR, false, true);
 
-   var = getenv("PGCLIENTENCODING");
-   if (var != NULL)
-       set_config_option("client_encoding", var, context,
+   envvar = getenv("PGCLIENTENCODING");
+   if (envvar != NULL)
+       set_config_option("client_encoding", envvar, PGC_POSTMASTER,
                          PGC_S_ENV_VAR, false, true);
 
+
+   /* If we got here all the options checked out okay, so apply them. */
+   for (item = head; item; item = item->next)
+   {
+       set_config_option(item->name, item->value, context,
+                         PGC_S_FILE, false, true);
+   }
+
  cleanup_list:
-   free(in_conffile);
    free_name_value_list(head);
+   if (cvc)
+       free(cvc);
 }
 
 
@@ -389,6 +407,7 @@ ParseConfigFile(const char *config_file, const char *calling_file,
    while ((token = yylex()))
    {
        char       *opt_name, *opt_value;
+       struct name_value_pair *item;
 
        if (token == GUC_EOL)   /* empty or comment line */
            continue;
@@ -421,7 +440,7 @@ ParseConfigFile(const char *config_file, const char *calling_file,
            goto parse_error;
 
        /* OK, process the option name and value */
-       if (pg_strcasecmp(opt_name, "include") == 0)
+       if (guc_name_compare(opt_name, "include") == 0)
        {
            /*
             * An include directive isn't a variable and should be processed
@@ -443,29 +462,39 @@ ParseConfigFile(const char *config_file, const char *calling_file,
            pfree(opt_name);
            pfree(opt_value);
        }
-       else if (pg_strcasecmp(opt_name, "custom_variable_classes") == 0)
+       else if (guc_name_compare(opt_name, "custom_variable_classes") == 0)
        {
            /*
             * This variable must be processed first as it controls
-            * the validity of other variables; so apply immediately.
+            * the validity of other variables; so it goes at the head
+            * of the result list.  If we already found a value for it,
+            * replace with this one.
             */
-           if (!set_config_option(opt_name, opt_value, context,
-                                  PGC_S_FILE, false, true))
+           item = *head_p;
+           if (item != NULL &&
+               guc_name_compare(item->name, "custom_variable_classes") == 0)
            {
-               pfree(opt_name);
-               pfree(opt_value);
-
-               /* We assume the error message was logged already. */
-               OK = false;
-               goto cleanup_exit;
+               /* replace existing head item */
+               pfree(item->name);
+               pfree(item->value);
+               item->name = opt_name;
+               item->value = opt_value;
+           }
+           else
+           {
+               /* prepend to list */
+               item = palloc(sizeof *item);
+               item->name = opt_name;
+               item->value = opt_value;
+               item->next = *head_p;
+               *head_p = item;
+               if (*tail_p == NULL)
+                   *tail_p = item;
            }
        }
-
-       if (pg_strcasecmp(opt_name, "include") != 0)
+       else
        {
-           /* append to list */
-           struct name_value_pair *item;
-
+           /* ordinary variable, append to list */
            item = palloc(sizeof *item);
            item->name = opt_name;
            item->value = opt_value;
index 60f7ed5ca32b6325f074a4be3ceef81f257da337..6cdc87df6db46939c2741699db34b0c557cfe246 100644 (file)
@@ -10,7 +10,7 @@
  * Written by Peter Eisentraut .
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/utils/misc/guc.c,v 1.416 2007/09/03 18:46:30 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/utils/misc/guc.c,v 1.417 2007/09/10 00:57:21 tgl Exp $
  *
  *--------------------------------------------------------------------
  */
@@ -2505,7 +2505,7 @@ static void ReportGUCOption(struct config_generic * record);
 static void ShowGUCConfigOption(const char *name, DestReceiver *dest);
 static void ShowAllGUCConfig(DestReceiver *dest);
 static char *_ShowOption(struct config_generic * record, bool use_units);
-static bool is_newvalue_equal(struct config_generic * record, const char *newvalue);
+static bool is_newvalue_equal(struct config_generic *record, const char *newvalue);
 
 
 /*
@@ -2690,40 +2690,6 @@ build_guc_variables(void)
          sizeof(struct config_generic *), guc_var_compare);
 }
 
-static bool
-is_custom_class(const char *name, int dotPos)
-{
-   /*
-    * assign_custom_variable_classes() has made sure no empty identifiers or
-    * whitespace exists in the variable
-    */
-   bool        result = false;
-   const char *ccs = GetConfigOption("custom_variable_classes");
-
-   if (ccs != NULL)
-   {
-       const char *start = ccs;
-
-       for (;; ++ccs)
-       {
-           int         c = *ccs;
-
-           if (c == 0 || c == ',')
-           {
-               if (dotPos == ccs - start && strncmp(start, name, dotPos) == 0)
-               {
-                   result = true;
-                   break;
-               }
-               if (c == 0)
-                   break;
-               start = ccs + 1;
-           }
-       }
-   }
-   return result;
-}
-
 /*
  * Add a new GUC variable to the list of known variables. The
  * list is expanded if needed.
@@ -2767,7 +2733,7 @@ add_guc_variable(struct config_generic * var, int elevel)
  * Create and add a placeholder variable. It's presumed to belong
  * to a valid custom variable class at this point.
  */
-static struct config_string *
+static struct config_generic *
 add_placeholder_variable(const char *name, int elevel)
 {
    size_t      sz = sizeof(struct config_string) + sizeof(char *);
@@ -2777,9 +2743,8 @@ add_placeholder_variable(const char *name, int elevel)
    var = (struct config_string *) guc_malloc(elevel, sz);
    if (var == NULL)
        return NULL;
-
-   gen = &var->gen;
    memset(var, 0, sz);
+   gen = &var->gen;
 
    gen->name = guc_strdup(elevel, name);
    if (gen->name == NULL)
@@ -2796,7 +2761,8 @@ add_placeholder_variable(const char *name, int elevel)
 
    /*
     * The char* is allocated at the end of the struct since we have no
-    * 'static' place to point to.
+    * 'static' place to point to.  Note that the current value, as well
+    * as the boot and reset values, start out NULL.
     */
    var->variable = (char **) (var + 1);
 
@@ -2807,17 +2773,53 @@ add_placeholder_variable(const char *name, int elevel)
        return NULL;
    }
 
-   return var;
+   return gen;
+}
+
+/*
+ * Detect whether the portion of "name" before dotPos matches any custom
+ * variable class name listed in custom_var_classes.  The latter must be
+ * formatted the way that assign_custom_variable_classes does it, ie,
+ * no whitespace.  NULL is valid for custom_var_classes.
+ */
+static bool
+is_custom_class(const char *name, int dotPos, const char *custom_var_classes)
+{
+   bool        result = false;
+   const char *ccs = custom_var_classes;
+
+   if (ccs != NULL)
+   {
+       const char *start = ccs;
+
+       for (;; ++ccs)
+       {
+           char        c = *ccs;
+
+           if (c == '\0' || c == ',')
+           {
+               if (dotPos == ccs - start && strncmp(start, name, dotPos) == 0)
+               {
+                   result = true;
+                   break;
+               }
+               if (c == '\0')
+                   break;
+               start = ccs + 1;
+           }
+       }
+   }
+   return result;
 }
 
 /*
- * Look up option NAME. If it exists, return a pointer to its record,
- * else return NULL.
+ * Look up option NAME.  If it exists, return a pointer to its record,
+ * else return NULL.  If create_placeholders is TRUE, we'll create a
+ * placeholder record for a valid-looking custom variable name.
  */
 static struct config_generic *
-find_option(const char *name, int elevel)
+find_option(const char *name, bool create_placeholders, int elevel)
 {
-   const char *dot;
    const char **key = &name;
    struct config_generic **res;
    int         i;
@@ -2844,17 +2846,21 @@ find_option(const char *name, int elevel)
    for (i = 0; map_old_guc_names[i] != NULL; i += 2)
    {
        if (guc_name_compare(name, map_old_guc_names[i]) == 0)
-           return find_option(map_old_guc_names[i + 1], elevel);
+           return find_option(map_old_guc_names[i + 1], false, elevel);
    }
 
-   /*
-    * Check if the name is qualified, and if so, check if the qualifier maps
-    * to a custom variable class.
-    */
-   dot = strchr(name, GUC_QUALIFIER_SEPARATOR);
-   if (dot != NULL && is_custom_class(name, dot - name))
-       /* Add a placeholder variable for this name */
-       return (struct config_generic *) add_placeholder_variable(name, elevel);
+   if (create_placeholders)
+   {
+       /*
+        * Check if the name is qualified, and if so, check if the qualifier
+        * matches any custom variable class.  If so, add a placeholder.
+        */
+       const char *dot = strchr(name, GUC_QUALIFIER_SEPARATOR);
+
+       if (dot != NULL &&
+           is_custom_class(name, dot - name, custom_variable_classes))
+           return add_placeholder_variable(name, elevel);
+   }
 
    /* Unknown name */
    return NULL;
@@ -2902,30 +2908,6 @@ guc_name_compare(const char *namea, const char *nameb)
 }
 
 
-static int
-guc_get_index(const char *name)
-{
-   const char **key = &name;
-   struct config_generic **res;
-
-   Assert(name);
-
-   /*
-    * By equating const char ** with struct config_generic *, we are assuming
-    * the name field is first in config_generic.
-    */
-   res = (struct config_generic **) bsearch((void *) &key,
-                                            (void *) guc_variables,
-                                            num_guc_variables,
-                                            sizeof(struct config_generic *),
-                                            guc_var_compare);
-   if (!res)
-       return -1;
-
-   return res - guc_variables;
-}
-
-
 /*
  * Initialize GUC options during program startup.
  *
@@ -3017,7 +2999,7 @@ InitializeGUCOptions(void)
 
                    if (conf->boot_val == NULL)
                    {
-                       /* Cannot set value yet */
+                       /* leave the value NULL, do not call assign hook */
                        break;
                    }
 
@@ -3067,7 +3049,8 @@ InitializeGUCOptions(void)
 
    /*
     * For historical reasons, some GUC parameters can receive defaults from
-    * environment variables.  Process those settings.
+    * environment variables.  Process those settings.  NB: if you add or
+    * remove anything here, see also ProcessConfigFile().
     */
 
    env = getenv("PGPORT");
@@ -3335,16 +3318,10 @@ ResetAllOptions(void)
                    struct config_string *conf = (struct config_string *) gconf;
                    char       *str;
 
-                   if (conf->reset_val == NULL)
-                   {
-                       /* Nothing to reset to, as yet; so do nothing */
-                       break;
-                   }
-
                    /* We need not strdup here */
                    str = conf->reset_val;
 
-                   if (conf->assign_hook)
+                   if (conf->assign_hook && str)
                    {
                        const char *newstr;
 
@@ -3517,7 +3494,9 @@ AtEOXact_GUC(bool isCommit, int nestLevel)
        /*
         * Skip if nothing's happened to this var in this transaction
         */
-       if (my_status == 0)
+       if ((my_status & (GUC_HAVE_TENTATIVE |
+                         GUC_HAVE_LOCAL |
+                         GUC_HAVE_STACK)) == 0)
        {
            Assert(stack == NULL);
            continue;
@@ -3679,7 +3658,7 @@ AtEOXact_GUC(bool isCommit, int nestLevel)
 
                    if (*conf->variable != newval)
                    {
-                       if (conf->assign_hook)
+                       if (conf->assign_hook && newval)
                        {
                            const char *newstr;
 
@@ -4182,7 +4161,7 @@ set_config_option(const char *name, const char *value,
    else
        elevel = ERROR;
 
-   record = find_option(name, elevel);
+   record = find_option(name, true, elevel);
    if (record == NULL)
    {
        ereport(elevel,
@@ -4192,11 +4171,13 @@ set_config_option(const char *name, const char *value,
    }
 
    /*
-    * Do not replace a value that has been set on the command line by a SIGHUP
-    * reload
+    * If source is postgresql.conf, mark the found record with GUC_IS_IN_FILE.
+    * This is for the convenience of ProcessConfigFile.  Note that we do it
+    * even if changeVal is false, since ProcessConfigFile wants the marking
+    * to occur during its testing pass.
     */
-   if (context == PGC_SIGHUP && record->source == PGC_S_ARGV)
-       return true;
+   if (source == PGC_S_FILE)
+       record->status |= GUC_IS_IN_FILE;
 
    /*
     * Check if the option can be set at this time. See guc.h for the precise
@@ -4220,12 +4201,17 @@ set_config_option(const char *name, const char *value,
        case PGC_POSTMASTER:
            if (context == PGC_SIGHUP)
            {
+               /*
+                * We are reading a PGC_POSTMASTER var from postgresql.conf.
+                * We can't change the setting, so give a warning if the DBA
+                * tries to change it.  (Throwing an error would be more
+                * consistent, but seems overly rigid.)
+                */
                if (changeVal && !is_newvalue_equal(record, value))
                    ereport(elevel,
                            (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
                             errmsg("parameter \"%s\" cannot be changed after server start; configuration file change ignored",
                                    name)));
-
                return true;
            }
            if (context != PGC_POSTMASTER)
@@ -4298,22 +4284,17 @@ set_config_option(const char *name, const char *value,
     * value from the database's/user's/client's default settings or
     * when we reset a value to its default.
     */
-   makeDefault = changeVal && (source <= PGC_S_OVERRIDE)
-                   && ((value != NULL) || source == PGC_S_DEFAULT);
+   makeDefault = changeVal && (source <= PGC_S_OVERRIDE) &&
+       ((value != NULL) || source == PGC_S_DEFAULT);
 
    /*
-    * Ignore attempted set if overridden by previously processed
-    * setting.  However, if changeVal is false then plow ahead anyway
-    * since we are trying to find out if the value is potentially
-    * good, not actually use it.  Also keep going if makeDefault is
-    * true, since we may want to set the reset/stacked values even if
-    * we can't set the variable itself.  There's one exception to
-    * this rule: if we want to apply the default value to variables
-    * that were removed from the configuration file.  This is
-    * indicated by source == PGC_S_DEFAULT and context == PGC_SIGHUP.
+    * Ignore attempted set if overridden by previously processed setting.
+    * However, if changeVal is false then plow ahead anyway since we are
+    * trying to find out if the value is potentially good, not actually use
+    * it. Also keep going if makeDefault is true, since we may want to set
+    * the reset/stacked values even if we can't set the variable itself.
     */
-   if (record->source > source
-       && !(source == PGC_S_DEFAULT && context == PGC_SIGHUP))
+   if (record->source > source)
    {
        if (changeVal && !makeDefault)
        {
@@ -4326,6 +4307,9 @@ set_config_option(const char *name, const char *value,
 
    /*
     * Evaluate value and set variable.
+    *
+    * Note: if value == NULL then we are supposed to set to the reset_val,
+    * except when source == PGC_S_DEFAULT; then we set to the boot_val.
     */
    switch (record->vartype)
    {
@@ -4345,14 +4329,8 @@ set_config_option(const char *name, const char *value,
                        return false;
                    }
                }
-               /*
-                * If value == NULL and source == PGC_S_DEFAULT then
-                * we reset some value to its default (removed from
-                * configuration file).
-                */
                else if (source == PGC_S_DEFAULT)
                    newval = conf->boot_val;
-               /* else we handle a "RESET varname" command */
                else
                {
                    newval = conf->reset_val;
@@ -4440,14 +4418,8 @@ set_config_option(const char *name, const char *value,
                        return false;
                    }
                }
-               /*
-                * If value == NULL and source == PGC_S_DEFAULT then
-                * we reset some value to its default (removed from
-                * configuration file).
-                */
                else if (source == PGC_S_DEFAULT)
                    newval = conf->boot_val;
-               /* else we handle a "RESET varname" command */
                else
                {
                    newval = conf->reset_val;
@@ -4532,14 +4504,8 @@ set_config_option(const char *name, const char *value,
                        return false;
                    }
                }
-               /*
-                * If value == NULL and source == PGC_S_DEFAULT then
-                * we reset some value to its default (removed from
-                * configuration file).
-                */
                else if (source == PGC_S_DEFAULT)
                    newval = conf->boot_val;
-               /* else we handle a "RESET varname" command */
                else
                {
                    newval = conf->reset_val;
@@ -4618,11 +4584,6 @@ set_config_option(const char *name, const char *value,
                    if (conf->gen.flags & GUC_IS_NAME)
                        truncate_identifier(newval, strlen(newval), true);
                }
-               /*
-                * If value == NULL and source == PGC_S_DEFAULT then
-                * we reset some value to its default (removed from
-                * configuration file).
-                */
                else if (source == PGC_S_DEFAULT)
                {
                    if (conf->boot_val == NULL)
@@ -4634,25 +4595,25 @@ set_config_option(const char *name, const char *value,
                            return false;
                    }
                }
-               /* else we handle a "RESET varname" command */
-               else if (conf->reset_val)
+               else
                {
                    /*
                     * We could possibly avoid strdup here, but easier to make
-                    * this case work the same as the normal assignment case.
+                    * this case work the same as the normal assignment case;
+                    * note the possible free of newval below.
                     */
-                   newval = guc_strdup(elevel, conf->reset_val);
-                   if (newval == NULL)
-                       return false;
+                   if (conf->reset_val == NULL)
+                       newval = NULL;
+                   else
+                   {
+                       newval = guc_strdup(elevel, conf->reset_val);
+                       if (newval == NULL)
+                           return false;
+                   }
                    source = conf->gen.reset_source;
                }
-               else
-               {
-                   /* Nothing to reset to, as yet; so do nothing */
-                   break;
-               }
 
-               if (conf->assign_hook)
+               if (conf->assign_hook && newval)
                {
                    const char *hookresult;
 
@@ -4718,7 +4679,7 @@ set_config_option(const char *name, const char *value,
                            }
                        }
                        /* Perhaps we didn't install newval anywhere */
-                       if (!string_field_used(conf, newval))
+                       if (newval && !string_field_used(conf, newval))
                            free(newval);
                    }
                    else if (isLocal)
@@ -4734,7 +4695,7 @@ set_config_option(const char *name, const char *value,
                        guc_dirty = true;
                    }
                }
-               else
+               else if (newval)
                    free(newval);
                break;
            }
@@ -4774,7 +4735,7 @@ GetConfigOption(const char *name)
    struct config_generic *record;
    static char buffer[256];
 
-   record = find_option(name, ERROR);
+   record = find_option(name, false, ERROR);
    if (record == NULL)
        ereport(ERROR,
                (errcode(ERRCODE_UNDEFINED_OBJECT),
@@ -4807,6 +4768,10 @@ GetConfigOption(const char *name)
 
 /*
  * Get the RESET value associated with the given option.
+ *
+ * Note: this is not re-entrant, due to use of static result buffer;
+ * not to mention that a string variable could have its reset_val changed.
+ * Beware of assuming the result value is good for very long.
  */
 const char *
 GetConfigOptionResetString(const char *name)
@@ -4814,7 +4779,7 @@ GetConfigOptionResetString(const char *name)
    struct config_generic *record;
    static char buffer[256];
 
-   record = find_option(name, ERROR);
+   record = find_option(name, false, ERROR);
    if (record == NULL)
        ereport(ERROR,
                (errcode(ERRCODE_UNDEFINED_OBJECT),
@@ -4854,7 +4819,7 @@ IsSuperuserConfigOption(const char *name)
 {
    struct config_generic *record;
 
-   record = find_option(name, ERROR);
+   record = find_option(name, false, ERROR);
    /* On an unrecognized name, don't error, just return false. */
    if (record == NULL)
        return false;
@@ -4886,7 +4851,7 @@ flatten_set_variable_args(const char *name, List *args)
        return NULL;
 
    /* Else get flags for the variable */
-   record = find_option(name, ERROR);
+   record = find_option(name, true, ERROR);
    if (record == NULL)
        ereport(ERROR,
                (errcode(ERRCODE_UNDEFINED_OBJECT),
@@ -5145,22 +5110,54 @@ set_config_by_name(PG_FUNCTION_ARGS)
 }
 
 
+/*
+ * Common code for DefineCustomXXXVariable subroutines: allocate the
+ * new variable's config struct and fill in generic fields.
+ */
+static struct config_generic *
+init_custom_variable(const char *name,
+                    const char *short_desc,
+                    const char *long_desc,
+                    GucContext context,
+                    enum config_type type,
+                    size_t sz)
+{
+   struct config_generic *gen;
+
+   gen = (struct config_generic *) guc_malloc(ERROR, sz);
+   memset(gen, 0, sz);
+
+   gen->name = guc_strdup(ERROR, name);
+   gen->context = context;
+   gen->group = CUSTOM_OPTIONS;
+   gen->short_desc = short_desc;
+   gen->long_desc = long_desc;
+   gen->vartype = type;
+
+   return gen;
+}
+
+/*
+ * Common code for DefineCustomXXXVariable subroutines: insert the new
+ * variable into the GUC variable array, replacing any placeholder.
+ */
 static void
-define_custom_variable(struct config_generic * variable)
+define_custom_variable(struct config_generic *variable)
 {
    const char *name = variable->name;
    const char **nameAddr = &name;
    const char *value;
    struct config_string *pHolder;
-   struct config_generic **res = (struct config_generic **) bsearch(
-                                                         (void *) &nameAddr,
-                                                     (void *) guc_variables,
-                                                          num_guc_variables,
-                                            sizeof(struct config_generic *),
-                                                           guc_var_compare);
+   struct config_generic **res;
 
+   res = (struct config_generic **) bsearch((void *) &nameAddr,
+                                            (void *) guc_variables,
+                                            num_guc_variables,
+                                            sizeof(struct config_generic *),
+                                            guc_var_compare);
    if (res == NULL)
    {
+       /* No placeholder to replace, so just add it */
        add_guc_variable(variable, ERROR);
        return;
    }
@@ -5174,13 +5171,14 @@ define_custom_variable(struct config_generic * variable)
                 errmsg("attempt to redefine parameter \"%s\"", name)));
 
    Assert((*res)->vartype == PGC_STRING);
-   pHolder = (struct config_string *) * res;
+   pHolder = (struct config_string *) (*res);
 
-   /* We have the same name, no sorting is necessary */
+   /*
+    * Replace the placeholder.
+    * We aren't changing the name, so no re-sorting is necessary
+    */
    *res = variable;
 
-   value = *pHolder->variable;
-
    /*
     * Assign the string value stored in the placeholder to the real variable.
     *
@@ -5188,9 +5186,12 @@ define_custom_variable(struct config_generic * variable)
     * assignment, since we don't want it to roll back if the current xact
     * fails later.
     */
-   set_config_option(name, value,
-                     pHolder->gen.context, pHolder->gen.source,
-                     false, true);
+   value = *pHolder->variable;
+
+   if (value)
+       set_config_option(name, value,
+                         pHolder->gen.context, pHolder->gen.source,
+                         false, true);
 
    /*
     * Free up as much as we conveniently can of the placeholder structure
@@ -5203,22 +5204,6 @@ define_custom_variable(struct config_generic * variable)
    free(pHolder);
 }
 
-static void
-init_custom_variable(struct config_generic * gen,
-                    const char *name,
-                    const char *short_desc,
-                    const char *long_desc,
-                    GucContext context,
-                    enum config_type type)
-{
-   gen->name = guc_strdup(ERROR, name);
-   gen->context = context;
-   gen->group = CUSTOM_OPTIONS;
-   gen->short_desc = short_desc;
-   gen->long_desc = long_desc;
-   gen->vartype = type;
-}
-
 void
 DefineCustomBoolVariable(const char *name,
                         const char *short_desc,
@@ -5228,13 +5213,13 @@ DefineCustomBoolVariable(const char *name,
                         GucBoolAssignHook assign_hook,
                         GucShowHook show_hook)
 {
-   size_t      sz = sizeof(struct config_bool);
-   struct config_bool *var = (struct config_bool *) guc_malloc(ERROR, sz);
-
-   memset(var, 0, sz);
-   init_custom_variable(&var->gen, name, short_desc, long_desc, context, PGC_BOOL);
+   struct config_bool *var;
 
+   var = (struct config_bool *)
+       init_custom_variable(name, short_desc, long_desc, context,
+                            PGC_BOOL, sizeof(struct config_bool));
    var->variable = valueAddr;
+   var->boot_val = *valueAddr;
    var->reset_val = *valueAddr;
    var->assign_hook = assign_hook;
    var->show_hook = show_hook;
@@ -5252,13 +5237,13 @@ DefineCustomIntVariable(const char *name,
                        GucIntAssignHook assign_hook,
                        GucShowHook show_hook)
 {
-   size_t      sz = sizeof(struct config_int);
-   struct config_int *var = (struct config_int *) guc_malloc(ERROR, sz);
-
-   memset(var, 0, sz);
-   init_custom_variable(&var->gen, name, short_desc, long_desc, context, PGC_INT);
+   struct config_int *var;
 
+   var = (struct config_int *)
+       init_custom_variable(name, short_desc, long_desc, context,
+                            PGC_INT, sizeof(struct config_int));
    var->variable = valueAddr;
+   var->boot_val = *valueAddr;
    var->reset_val = *valueAddr;
    var->min = minValue;
    var->max = maxValue;
@@ -5278,13 +5263,13 @@ DefineCustomRealVariable(const char *name,
                         GucRealAssignHook assign_hook,
                         GucShowHook show_hook)
 {
-   size_t      sz = sizeof(struct config_real);
-   struct config_real *var = (struct config_real *) guc_malloc(ERROR, sz);
-
-   memset(var, 0, sz);
-   init_custom_variable(&var->gen, name, short_desc, long_desc, context, PGC_REAL);
+   struct config_real *var;
 
+   var = (struct config_real *)
+       init_custom_variable(name, short_desc, long_desc, context,
+                            PGC_REAL, sizeof(struct config_real));
    var->variable = valueAddr;
+   var->boot_val = *valueAddr;
    var->reset_val = *valueAddr;
    var->min = minValue;
    var->max = maxValue;
@@ -5302,14 +5287,16 @@ DefineCustomStringVariable(const char *name,
                           GucStringAssignHook assign_hook,
                           GucShowHook show_hook)
 {
-   size_t      sz = sizeof(struct config_string);
-   struct config_string *var = (struct config_string *) guc_malloc(ERROR, sz);
-
-   memset(var, 0, sz);
-   init_custom_variable(&var->gen, name, short_desc, long_desc, context, PGC_STRING);
+   struct config_string *var;
 
+   var = (struct config_string *)
+       init_custom_variable(name, short_desc, long_desc, context,
+                            PGC_STRING, sizeof(struct config_string));
    var->variable = valueAddr;
-   var->reset_val = *valueAddr;
+   var->boot_val = *valueAddr;
+   /* we could probably do without strdup, but keep it like normal case */
+   if (var->boot_val)
+       var->reset_val = guc_strdup(ERROR, var->boot_val);
    var->assign_hook = assign_hook;
    var->show_hook = show_hook;
    define_custom_variable(&var->gen);
@@ -5345,7 +5332,7 @@ EmitWarningsOnPlaceholders(const char *className)
 void
 GetPGVariable(const char *name, DestReceiver *dest)
 {
-   if (pg_strcasecmp(name, "all") == 0)
+   if (guc_name_compare(name, "all") == 0)
        ShowAllGUCConfig(dest);
    else
        ShowGUCConfigOption(name, dest);
@@ -5356,7 +5343,7 @@ GetPGVariableResultDesc(const char *name)
 {
    TupleDesc   tupdesc;
 
-   if (pg_strcasecmp(name, "all") == 0)
+   if (guc_name_compare(name, "all") == 0)
    {
        /* need a tuple descriptor representing three TEXT columns */
        tupdesc = CreateTemplateTupleDesc(3, false);
@@ -5470,7 +5457,7 @@ GetConfigOptionByName(const char *name, const char **varname)
 {
    struct config_generic *record;
 
-   record = find_option(name, ERROR);
+   record = find_option(name, false, ERROR);
    if (record == NULL)
        ereport(ERROR,
                (errcode(ERRCODE_UNDEFINED_OBJECT),
@@ -5927,9 +5914,19 @@ _ShowOption(struct config_generic * record, bool use_units)
 }
 
 
+/*
+ * Attempt (badly) to detect if a proposed new GUC setting is the same
+ * as the current value.
+ *
+ * XXX this does not really work because it doesn't account for the
+ * effects of canonicalization of string values by assign_hooks.
+ */
 static bool
-is_newvalue_equal(struct config_generic * record, const char *newvalue)
+is_newvalue_equal(struct config_generic *record, const char *newvalue)
 {
+   /* newvalue == NULL isn't supported */
+   Assert(newvalue != NULL);
+
    switch (record->vartype)
    {
        case PGC_BOOL:
@@ -5960,7 +5957,8 @@ is_newvalue_equal(struct config_generic * record, const char *newvalue)
            {
                struct config_string *conf = (struct config_string *) record;
 
-               return strcmp(*conf->variable, newvalue) == 0;
+               return *conf->variable != NULL &&
+                   strcmp(*conf->variable, newvalue) == 0;
            }
    }
 
@@ -6140,7 +6138,7 @@ read_nondefault_variables(void)
        if ((varname = read_string_with_null(fp)) == NULL)
            break;
 
-       if ((record = find_option(varname, FATAL)) == NULL)
+       if ((record = find_option(varname, true, FATAL)) == NULL)
            elog(FATAL, "failed to locate variable %s in exec config params file", varname);
        if ((varvalue = read_string_with_null(fp)) == NULL)
            elog(FATAL, "invalid format of exec config params file");
@@ -6576,8 +6574,7 @@ assign_session_replication_role(const char *newval, bool doit, GucSource source)
 }
 
 static const char *
-assign_log_min_messages(const char *newval,
-                       bool doit, GucSource source)
+assign_log_min_messages(const char *newval, bool doit, GucSource source)
 {
    return (assign_msglvl(&log_min_messages, newval, doit, source));
 }
@@ -6756,23 +6753,16 @@ assign_custom_variable_classes(const char *newval, bool doit, GucSource source)
 {
    /*
     * Check syntax. newval must be a comma separated list of identifiers.
-    * Whitespace is allowed but skipped.
+    * Whitespace is allowed but removed from the result.
     */
    bool        hasSpaceAfterToken = false;
    const char *cp = newval;
    int         symLen = 0;
-   int         c;
+   char        c;
    StringInfoData buf;
 
-   /*
-    * Resetting custom_variable_classes by removing it from the
-    * configuration file will lead to newval = NULL
-    */
-   if (newval == NULL)
-       return guc_strdup(ERROR, "");
-
    initStringInfo(&buf);
-   while ((c = *cp++) != 0)
+   while ((c = *cp++) != '\0')
    {
        if (isspace((unsigned char) c))
        {
@@ -6783,12 +6773,12 @@ assign_custom_variable_classes(const char *newval, bool doit, GucSource source)
 
        if (c == ',')
        {
-           hasSpaceAfterToken = false;
-           if (symLen > 0)
+           if (symLen > 0)     /* terminate identifier */
            {
-               symLen = 0;
                appendStringInfoChar(&buf, ',');
+               symLen = 0;
            }
+           hasSpaceAfterToken = false;
            continue;
        }
 
@@ -6798,24 +6788,19 @@ assign_custom_variable_classes(const char *newval, bool doit, GucSource source)
             * Syntax error due to token following space after token or non
             * alpha numeric character
             */
-           ereport(LOG,
-                   (errcode(ERRCODE_SYNTAX_ERROR),
-                    errmsg("invalid syntax for \"custom_variable_classes\": \"%s\"", newval)));
            pfree(buf.data);
            return NULL;
        }
+       appendStringInfoChar(&buf, c);
        symLen++;
-       appendStringInfoChar(&buf, (char) c);
    }
 
    /* Remove stray ',' at end */
    if (symLen == 0 && buf.len > 0)
        buf.data[--buf.len] = '\0';
 
-   if (buf.len == 0)
-       newval = NULL;
-   else if (doit)
-       newval = guc_strdup(ERROR, buf.data);
+   /* GUC wants the result malloc'd */
+   newval = guc_strdup(LOG, buf.data);
 
    pfree(buf.data);
    return newval;
index f68a814f799616f3e935c1c350800b0d141556b0..0bc74ccdd0bd5238b94405768215dd07bed37761 100644 (file)
@@ -7,7 +7,7 @@
  *
  * Portions Copyright (c) 1996-2007, PostgreSQL Global Development Group
  *
- *   $PostgreSQL: pgsql/src/include/utils/guc_tables.h,v 1.33 2007/04/21 20:02:41 petere Exp $
+ *   $PostgreSQL: pgsql/src/include/utils/guc_tables.h,v 1.34 2007/09/10 00:57:22 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -130,7 +130,7 @@ struct config_generic
 #define GUC_SUPERUSER_ONLY     0x0100  /* show only to superusers */
 #define GUC_IS_NAME                0x0200  /* limit string to NAMEDATALEN-1 */
 
-#define GUC_UNIT_KB                0x0400  /* value is in 1 kB */
+#define GUC_UNIT_KB                0x0400  /* value is in kilobytes */
 #define GUC_UNIT_BLOCKS            0x0800  /* value is in blocks */
 #define GUC_UNIT_XBLOCKS       0x0C00  /* value is in xlog blocks */
 #define GUC_UNIT_MEMORY            0x0C00  /* mask for KB, BLOCKS, XBLOCKS */
@@ -144,6 +144,11 @@ struct config_generic
 #define GUC_HAVE_TENTATIVE 0x0001      /* tentative value is defined */
 #define GUC_HAVE_LOCAL     0x0002      /* a SET LOCAL has been executed */
 #define GUC_HAVE_STACK     0x0004      /* we have stacked prior value(s) */
+#define GUC_IS_IN_FILE     0x0008      /* found it in config file */
+/*
+ * Caution: the GUC_IS_IN_FILE bit is transient state for ProcessConfigFile.
+ * Do not assume that its value represents useful information elsewhere.
+ */
 
 
 /* GUC records for specific variable types */
@@ -151,8 +156,7 @@ struct config_generic
 struct config_bool
 {
    struct config_generic gen;
-   /* these fields must be set correctly in initial value: */
-   /* (all but reset_val are constants) */
+   /* constant fields, must be set correctly in initial value: */
    bool       *variable;
    bool        boot_val;
    GucBoolAssignHook assign_hook;
@@ -165,8 +169,7 @@ struct config_bool
 struct config_int
 {
    struct config_generic gen;
-   /* these fields must be set correctly in initial value: */
-   /* (all but reset_val are constants) */
+   /* constant fields, must be set correctly in initial value: */
    int        *variable;
    int         boot_val;
    int         min;
@@ -181,8 +184,7 @@ struct config_int
 struct config_real
 {
    struct config_generic gen;
-   /* these fields must be set correctly in initial value: */
-   /* (all but reset_val are constants) */
+   /* constant fields, must be set correctly in initial value: */
    double     *variable;
    double      boot_val;
    double      min;
@@ -197,8 +199,7 @@ struct config_real
 struct config_string
 {
    struct config_generic gen;
-   /* these fields must be set correctly in initial value: */
-   /* (all are constants) */
+   /* constant fields, must be set correctly in initial value: */
    char      **variable;
    const char *boot_val;
    GucStringAssignHook assign_hook;