Code review for GUC revert-values-if-removed-from-postgresql.conf patch;

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

@@ -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

src/backend/utils/misc/README

@@ -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

src/backend/utils/misc/guc-file.l

@@ -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;

src/backend/utils/misc/guc.c

@@ -10,7 +10,7 @@
  * Written by Peter Eisentraut <peter_e@gmx.net>.
  *
  * 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;

src/include/utils/guc_tables.h

@@ -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;