Restructure error handling in reading of postgresql.conf.

This patch has two distinct purposes: to report multiple problems in
postgresql.conf rather than always bailing out after the first one,
and to change the policy for whether changes are applied when there are
unrelated errors in postgresql.conf.

Formerly the policy was to apply no changes if any errors could be
detected, but that had a significant consistency problem, because in some
cases specific values might be seen as valid by some processes but invalid
by others.  This meant that the latter processes would fail to adopt
changes in other parameters even though the former processes had done so.

The new policy is that during SIGHUP, the file is rejected as a whole
if there are any errors in the "name = value" syntax, or if any lines
attempt to set nonexistent built-in parameters, or if any lines attempt
to set custom parameters whose prefix is not listed in (the new value of)
custom_variable_classes.  These tests should always give the same results
in all processes, and provide what seems a reasonably robust defense
against loading values from badly corrupted config files.  If these tests
pass, all processes will apply all settings that they individually see as
good, ignoring (but logging) any they don't.

In addition, the postmaster does not abandon reading a configuration file
after the first syntax error, but continues to read the file and report
syntax errors (up to a maximum of 100 syntax errors per file).

The postmaster will still refuse to start up if the configuration file
contains any errors at startup time, but these changes allow multiple
errors to be detected and reported before quitting.

Alexey Klyukin, reviewed by Andy Colson and av (Alexander ?)
with some additional hacking by Tom Lane

doc/src/sgml/config.sgml

@@ -101,7 +101,9 @@ include 'filename'
     value. Alternatively, you can send the signal to a single server
     process directly.  Some parameters can only be set at server start;
     any changes to their entries in the configuration file will be ignored
-    until the server is restarted.
+    until the server is restarted.  Invalid parameter settings in the
+    configuration file are likewise ignored (but logged) during
+    <systemitem>SIGHUP</> processing.
    </para>
 
    <para>

src/backend/access/transam/xlog.c

@@ -5295,10 +5295,12 @@ readRecoveryCommandFile(void)
 	}
 
 	/*
-	 * Since we're asking ParseConfigFp() to error out at FATAL, there's no
-	 * need to check the return value.
+	 * Since we're asking ParseConfigFp() to report errors as FATAL, there's
+	 * no need to check the return value.
 	 */
-	ParseConfigFp(fd, RECOVERY_COMMAND_FILE, 0, FATAL, &head, &tail);
+	(void) ParseConfigFp(fd, RECOVERY_COMMAND_FILE, 0, FATAL, &head, &tail);
+
+	FreeFile(fd);
 
 	for (item = head; item; item = item->next)
 	{
@@ -5504,7 +5506,6 @@ readRecoveryCommandFile(void)
 	}
 
 	FreeConfigVariables(head);
-	FreeFile(fd);
 }
 
 /*

src/backend/commands/extension.c

@@ -472,9 +472,10 @@ parse_extension_control_file(ExtensionControlFile *control,
 	}
 
 	/*
-	 * Parse the file content, using GUC's file parsing code
+	 * Parse the file content, using GUC's file parsing code.  We need not
+	 * check the return value since any errors will be thrown at ERROR level.
 	 */
-	ParseConfigFp(file, filename, 0, ERROR, &head, &tail);
+	(void) ParseConfigFp(file, filename, 0, ERROR, &head, &tail);
 
 	FreeFile(file);
 

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

@@ -105,6 +105,8 @@ STRING          \'([^'\\\n]|\\.|\'\')*\'
 void
 ProcessConfigFile(GucContext context)
 {
+	bool		error = false;
+	bool		apply = false;
 	int			elevel;
 	ConfigVariable *item,
 				   *head,
@@ -113,24 +115,28 @@ ProcessConfigFile(GucContext context)
 	struct config_string *cvc_struct;
 	int			i;
 
-	Assert(context == PGC_POSTMASTER || context == PGC_SIGHUP);
+	/*
+	 * Config files are processed on startup (by the postmaster only)
+	 * and on SIGHUP (by the postmaster and its children)
+	 */
+	Assert((context == PGC_POSTMASTER && !IsUnderPostmaster) ||
+		   context == PGC_SIGHUP);
 
-	if (context == PGC_SIGHUP)
-	{
-		/*
-		 * To avoid cluttering the log, only the postmaster bleats loudly
-		 * about problems with the config file.
-		 */
-		elevel = IsUnderPostmaster ? DEBUG2 : LOG;
-	}
-	else
-		elevel = ERROR;
+	/*
+	 * To avoid cluttering the log, only the postmaster bleats loudly
+	 * about problems with the config file.
+	 */
+	elevel = IsUnderPostmaster ? DEBUG2 : LOG;
 
 	/* Parse the file into a list of option names and values */
 	head = tail = NULL;
 
 	if (!ParseConfigFile(ConfigFileName, NULL, 0, elevel, &head, &tail))
+	{
+		/* Syntax error(s) detected in the file, so bail out */
+		error = true;
 		goto cleanup_list;
+	}
 
 	/*
 	 * We need the proposed new value of custom_variable_classes to check
@@ -147,7 +153,10 @@ ProcessConfigFile(GucContext context)
 	{
 		cvc = guc_strdup(elevel, cvc_struct->reset_val);
 		if (cvc == NULL)
+		{
+			error = true;
 			goto cleanup_list;
+		}
 	}
 	else if (head != NULL &&
 			 guc_name_compare(head->name, "custom_variable_classes") == 0)
@@ -159,10 +168,16 @@ ProcessConfigFile(GucContext context)
 
 		cvc = guc_strdup(elevel, head->value);
 		if (cvc == NULL)
+		{
+			error = true;
 			goto cleanup_list;
+		}
 		if (!call_string_check_hook(cvc_struct, &cvc, &extra,
 									PGC_S_FILE, elevel))
+		{
+			error = true;
 			goto cleanup_list;
+		}
 		if (extra)
 			free(extra);
 	}
@@ -180,60 +195,69 @@ ProcessConfigFile(GucContext context)
 	}
 
 	/*
-	 * 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.
+	 * Check if all the supplied option names are valid, as an additional
+	 * quasi-syntactic check on the validity of the config file.  It is
+	 * important that the postmaster and all backends agree on the results
+	 * of this phase, else we will have strange inconsistencies about which
+	 * processes accept a config file update and which don't.  Hence, custom
+	 * variable names can only be checked against custom_variable_classes,
+	 * not against any loadable modules that might (or might not) be present.
+	 * Likewise, we don't attempt to validate the options' values here.
+	 *
+	 * In addition, the GUC_IS_IN_FILE flag is set on each existing GUC
+	 * variable mentioned in the file.
 	 */
 	for (item = head; item; item = item->next)
 	{
-		char *sep = strchr(item->name, GUC_QUALIFIER_SEPARATOR);
+		char   *sep = strchr(item->name, GUC_QUALIFIER_SEPARATOR);
+		struct config_generic *record;
 
 		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.
-			 */
+			/* Custom variable, so check against custom_variable_classes */
 			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)
+						 errmsg("unrecognized configuration parameter \"%s\" in file \"%s\" line %u",
+								item->name,
+								item->filename, item->sourceline)));
+				error = true;
 				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, GUC_ACTION_SET, false))
-			goto cleanup_list;
+		record = find_option(item->name, false, elevel);
+
+		if (record)
+			record->status |= GUC_IS_IN_FILE;
+		else if (!sep)
+		{
+			/* Invalid non-custom variable, so complain */
+			ereport(elevel,
+					(errcode(ERRCODE_UNDEFINED_OBJECT),
+					 errmsg("unrecognized configuration parameter \"%s\" in file \"%s\" line %u",
+							item->name,
+							item->filename, item->sourceline)));
+			error = true;
+		}
 	}
 
+	/*
+	 * If we've detected any errors so far, we don't want to risk applying
+	 * any changes.
+	 */
+	if (error)
+		goto cleanup_list;
+
+	/* Otherwise, set flag that we're beginning to apply changes */
+	apply = true;
+
 	/*
 	 * 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.)
+	 * report that and continue.
 	 */
 	for (i = 0; i < num_guc_variables; i++)
 	{
@@ -249,6 +273,7 @@ ProcessConfigFile(GucContext context)
 					(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
 					 errmsg("parameter \"%s\" cannot be changed without restarting the server",
 							gconf->name)));
+			error = true;
 			continue;
 		}
 
@@ -267,12 +292,16 @@ ProcessConfigFile(GucContext context)
 		}
 
 		/* Now we can re-apply the wired-in default (i.e., the boot_val) */
-		set_config_option(gconf->name, NULL, context, PGC_S_DEFAULT,
-						  GUC_ACTION_SET, true);
-		if (context == PGC_SIGHUP)
-			ereport(elevel,
-					(errmsg("parameter \"%s\" removed from configuration file, reset to default",
-							gconf->name)));
+		if (set_config_option(gconf->name, NULL,
+							  context, PGC_S_DEFAULT,
+							  GUC_ACTION_SET, true) > 0)
+		{
+			/* Log the change if appropriate */
+			if (context == PGC_SIGHUP)
+				ereport(elevel,
+						(errmsg("parameter \"%s\" removed from configuration file, reset to default",
+								gconf->name)));
+		}
 	}
 
 	/*
@@ -298,12 +327,15 @@ ProcessConfigFile(GucContext context)
 						PGC_BACKEND, PGC_S_DYNAMIC_DEFAULT);
 	}
 
-	/* If we got here all the options checked out okay, so apply them. */
+	/*
+	 * Now apply the values from the config file.
+	 */
 	for (item = head; item; item = item->next)
 	{
 		char   *pre_value = NULL;
+		int		scres;
 
-		/* In SIGHUP cases in the postmaster, report changes */
+		/* In SIGHUP cases in the postmaster, we want to report changes */
 		if (context == PGC_SIGHUP && !IsUnderPostmaster)
 		{
 			const char *preval = GetConfigOption(item->name, true, false);
@@ -315,12 +347,16 @@ ProcessConfigFile(GucContext context)
 			pre_value = pstrdup(preval);
 		}
 
-		if (set_config_option(item->name, item->value, context,
-			   					 PGC_S_FILE, GUC_ACTION_SET, true))
+		scres = set_config_option(item->name, item->value,
+								  context, PGC_S_FILE,
+								  GUC_ACTION_SET, true);
+		if (scres > 0)
 		{
+			/* variable was updated, so remember the source location */
 			set_config_sourcefile(item->name, item->filename,
 								  item->sourceline);
 
+			/* and log the change if appropriate */
 			if (pre_value)
 			{
 				const char *post_value = GetConfigOption(item->name, true, false);
@@ -333,6 +369,9 @@ ProcessConfigFile(GucContext context)
 									item->name, item->value)));
 			}
 		}
+		else if (scres == 0)
+			error = true;
+		/* else no error but variable was not changed, do nothing */
 
 		if (pre_value)
 			pfree(pre_value);
@@ -345,11 +384,35 @@ ProcessConfigFile(GucContext context)
 	FreeConfigVariables(head);
 	if (cvc)
 		free(cvc);
+
+	if (error)
+	{
+		/* During postmaster startup, any error is fatal */
+		if (context == PGC_POSTMASTER)
+			ereport(ERROR,
+					(errcode(ERRCODE_CONFIG_FILE_ERROR),
+					 errmsg("configuration file \"%s\" contains errors",
+							ConfigFileName)));
+		else if (apply)
+			ereport(elevel,
+					(errcode(ERRCODE_CONFIG_FILE_ERROR),
+					 errmsg("configuration file \"%s\" contains errors; unaffected changes were applied",
+							ConfigFileName)));
+		else
+			ereport(elevel,
+					(errcode(ERRCODE_CONFIG_FILE_ERROR),
+					 errmsg("configuration file \"%s\" contains errors; no changes were applied",
+							ConfigFileName)));
+	}
 }
 
 /*
- * See next function for details. This one will just work with a config_file
- * name rather than an already opened File Descriptor
+ * Read and parse a single configuration file.  This function recurses
+ * to handle "include" directives.
+ *
+ * See ParseConfigFp for details.  This one merely adds opening the
+ * file rather than working from a caller-supplied file descriptor,
+ * and absolute-ifying the path name if necessary.
  */
 bool
 ParseConfigFile(const char *config_file, const char *calling_file,
@@ -423,9 +486,9 @@ ParseConfigFile(const char *config_file, const char *calling_file,
  *
  * Input parameters:
  *	fp: file pointer from AllocateFile for the configuration file to parse
- *	config_file: absolute or relative path of file to read
- *	depth: recursion depth (used only to prevent infinite recursion)
- *	elevel: error logging level determined by ProcessConfigFile()
+ *	config_file: absolute or relative path name of the configuration file
+ *	depth: recursion depth (should be 0 in the outermost call)
+ *	elevel: error logging level to use
  * Output parameters:
  *	head_p, tail_p: head and tail of linked list of name/value pairs
  *
@@ -435,13 +498,10 @@ ParseConfigFile(const char *config_file, const char *calling_file,
  *
  * Returns TRUE if successful, FALSE if an error occurred.  The error has
  * already been ereport'd, it is only necessary for the caller to clean up
- * its own state and release the name/value pairs list.
+ * its own state and release the ConfigVariable list.
  *
  * Note: if elevel >= ERROR then an error will not return control to the
- * caller, and internal state such as open files will not be cleaned up.
- * This case occurs only during postmaster or standalone-backend startup,
- * where an error will lead to immediate process exit anyway; so there is
- * no point in contorting the code so it can clean up nicely.
+ * caller, so there is no need to check the return value in that case.
  */
 bool
 ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
@@ -449,6 +509,7 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
 {
 	bool		OK = true;
 	YY_BUFFER_STATE lex_buffer;
+	int			errorcount;
 	int			token;
 
 	/*
@@ -458,11 +519,13 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
 	yy_switch_to_buffer(lex_buffer);
 
 	ConfigFileLineno = 1;
+	errorcount = 0;
 
 	/* This loop iterates once per logical line */
 	while ((token = yylex()))
 	{
-		char	   *opt_name, *opt_value;
+		char	   *opt_name = NULL;
+		char	   *opt_value = NULL;
 		ConfigVariable *item;
 
 		if (token == GUC_EOL)	/* empty or comment line */
@@ -512,12 +575,7 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
 			if (!ParseConfigFile(opt_value, config_file,
 								 depth + 1, elevel,
 								 head_p, tail_p))
-			{
-				pfree(opt_name);
-				pfree(opt_value);
 				OK = false;
-				goto cleanup_exit;
-			}
 			yy_switch_to_buffer(lex_buffer);
 			ConfigFileLineno = save_ConfigFileLineno;
 			pfree(opt_name);
@@ -576,25 +634,53 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
 		/* break out of loop if read EOF, else loop for next line */
 		if (token == 0)
 			break;
-	}
+		continue;
 
-	/* successful completion of parsing */
-	goto cleanup_exit;
+	parse_error:
+		/* release storage if we allocated any on this line */
+		if (opt_name)
+			pfree(opt_name);
+		if (opt_value)
+			pfree(opt_value);
 
- parse_error:
-	if (token == GUC_EOL || token == 0)
-		ereport(elevel,
-				(errcode(ERRCODE_SYNTAX_ERROR),
-				 errmsg("syntax error in file \"%s\" line %u, near end of line",
-						config_file, ConfigFileLineno - 1)));
-	else
-		ereport(elevel,
-				(errcode(ERRCODE_SYNTAX_ERROR),
-				 errmsg("syntax error in file \"%s\" line %u, near token \"%s\"",
-						config_file, ConfigFileLineno, yytext)));
-	OK = false;
+		/* report the error */
+		if (token == GUC_EOL || token == 0)
+			ereport(elevel,
+					(errcode(ERRCODE_SYNTAX_ERROR),
+					 errmsg("syntax error in file \"%s\" line %u, near end of line",
+							config_file, ConfigFileLineno - 1)));
+		else
+			ereport(elevel,
+					(errcode(ERRCODE_SYNTAX_ERROR),
+					 errmsg("syntax error in file \"%s\" line %u, near token \"%s\"",
+							config_file, ConfigFileLineno, yytext)));
+		OK = false;
+		errorcount++;
+
+		/*
+		 * To avoid producing too much noise when fed a totally bogus file,
+		 * give up after 100 syntax errors per file (an arbitrary number).
+		 * Also, if we're only logging the errors at DEBUG level anyway,
+		 * might as well give up immediately.  (This prevents postmaster
+		 * children from bloating the logs with duplicate complaints.)
+		 */
+		if (errorcount >= 100 || elevel <= DEBUG1)
+		{
+			ereport(elevel,
+					(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+					 errmsg("too many syntax errors found, abandoning file \"%s\"",
+							config_file)));
+			break;
+		}
+
+		/* resync to next end-of-line or EOF */
+		while (token != GUC_EOL && token != 0)
+			token = yylex();
+		/* break out of loop on EOF */
+		if (token == 0)
+			break;
+	}
 
-cleanup_exit:
 	yy_delete_buffer(lex_buffer);
 	return OK;
 }

src/backend/utils/misc/guc.c

@@ -5046,11 +5046,12 @@ config_enum_get_options(struct config_enum * record, const char *prefix,
 
 
 /*
- * Sets option `name' to given value. The value should be a string
- * which is going to be parsed and converted to the appropriate data
- * type.  The context and source parameters indicate in which context this
- * function is being called so it can apply the access restrictions
- * properly.
+ * Sets option `name' to given value.
+ *
+ * The value should be a string, which will be parsed and converted to
+ * the appropriate data type.  The context and source parameters indicate
+ * in which context this function is being called, so that it can apply the
+ * access restrictions properly.
  *
  * If value is NULL, set the option to its default value (normally the
  * reset_val, but if source == PGC_S_DEFAULT we instead use the boot_val).
@@ -5061,18 +5062,20 @@ config_enum_get_options(struct config_enum * record, const char *prefix,
  * If changeVal is false then don't really set the option but do all
  * the checks to see if it would work.
  *
+ * Return value:
+ *	+1: the value is valid and was successfully applied.
+ *	0:  the name or value is invalid (but see below).
+ *	-1: the value was not applied because of context, priority, or changeVal.
+ *
  * If there is an error (non-existing option, invalid value) then an
- * ereport(ERROR) is thrown *unless* this is called in a context where we
- * don't want to ereport (currently, startup or SIGHUP config file reread).
- * In that case we write a suitable error message via ereport(LOG) and
- * return false. This is working around the deficiencies in the ereport
- * mechanism, so don't blame me.  In all other cases, the function
- * returns true, including cases where the input is valid but we chose
- * not to apply it because of context or source-priority considerations.
+ * ereport(ERROR) is thrown *unless* this is called for a source for which
+ * we don't want an ERROR (currently, those are defaults, the config file,
+ * and per-database or per-user settings).  In those cases we write a
+ * suitable error message via ereport() and return 0.
  *
  * See also SetConfigOption for an external interface.
  */
-bool
+int
 set_config_option(const char *name, const char *value,
 				  GucContext context, GucSource source,
 				  GucAction action, bool changeVal)
@@ -5082,7 +5085,7 @@ set_config_option(const char *name, const char *value,
 	bool		prohibitValueChange = false;
 	bool		makeDefault;
 
-	if (context == PGC_SIGHUP || source == PGC_S_DEFAULT)
+	if (source == PGC_S_DEFAULT || source == PGC_S_FILE)
 	{
 		/*
 		 * To avoid cluttering the log, only the postmaster bleats loudly
@@ -5102,18 +5105,9 @@ set_config_option(const char *name, const char *value,
 		ereport(elevel,
 				(errcode(ERRCODE_UNDEFINED_OBJECT),
 			   errmsg("unrecognized configuration parameter \"%s\"", name)));
-		return false;
+		return 0;
 	}
 
-	/*
-	 * 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 (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
 	 * rules.
@@ -5121,22 +5115,13 @@ set_config_option(const char *name, const char *value,
 	switch (record->context)
 	{
 		case PGC_INTERNAL:
-			if (context == PGC_SIGHUP)
-			{
-				/*
-				 * Historically we've just silently ignored attempts to set
-				 * PGC_INTERNAL variables from the config file.  Maybe it'd be
-				 * better to use the prohibitValueChange logic for this?
-				 */
-				return true;
-			}
-			else if (context != PGC_INTERNAL)
+			if (context != PGC_INTERNAL)
 			{
 				ereport(elevel,
 						(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
 						 errmsg("parameter \"%s\" cannot be changed",
 								name)));
-				return false;
+				return 0;
 			}
 			break;
 		case PGC_POSTMASTER:
@@ -5150,13 +5135,7 @@ set_config_option(const char *name, const char *value,
 				 * hooks, etc, we can't just compare the given string directly
 				 * to what's stored.  Set a flag to check below after we have
 				 * the final storable value.
-				 *
-				 * During the "checking" pass we just do nothing, to avoid
-				 * printing the warning twice.
 				 */
-				if (!changeVal)
-					return true;
-
 				prohibitValueChange = true;
 			}
 			else if (context != PGC_POSTMASTER)
@@ -5165,7 +5144,7 @@ set_config_option(const char *name, const char *value,
 						(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
 						 errmsg("parameter \"%s\" cannot be changed without restarting the server",
 								name)));
-				return false;
+				return 0;
 			}
 			break;
 		case PGC_SIGHUP:
@@ -5175,7 +5154,7 @@ set_config_option(const char *name, const char *value,
 						(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
 						 errmsg("parameter \"%s\" cannot be changed now",
 								name)));
-				return false;
+				return 0;
 			}
 
 			/*
@@ -5197,7 +5176,7 @@ set_config_option(const char *name, const char *value,
 				 * backend start.
 				 */
 				if (IsUnderPostmaster)
-					return true;
+					return -1;
 			}
 			else if (context != PGC_POSTMASTER && context != PGC_BACKEND &&
 					 source != PGC_S_CLIENT)
@@ -5206,7 +5185,7 @@ set_config_option(const char *name, const char *value,
 						(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
 						 errmsg("parameter \"%s\" cannot be set after connection start",
 								name)));
-				return false;
+				return 0;
 			}
 			break;
 		case PGC_SUSET:
@@ -5216,7 +5195,7 @@ set_config_option(const char *name, const char *value,
 						(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 						 errmsg("permission denied to set parameter \"%s\"",
 								name)));
-				return false;
+				return 0;
 			}
 			break;
 		case PGC_USERSET:
@@ -5254,7 +5233,7 @@ set_config_option(const char *name, const char *value,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("cannot set parameter \"%s\" within security-definer function",
 							name)));
-			return false;
+			return 0;
 		}
 		if (InSecurityRestrictedOperation())
 		{
@@ -5262,7 +5241,7 @@ set_config_option(const char *name, const char *value,
 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 					 errmsg("cannot set parameter \"%s\" within security-restricted operation",
 							name)));
-			return false;
+			return 0;
 		}
 	}
 
@@ -5288,7 +5267,7 @@ set_config_option(const char *name, const char *value,
 		{
 			elog(DEBUG3, "\"%s\": setting ignored because previous source is higher priority",
 				 name);
-			return true;
+			return -1;
 		}
 		changeVal = false;
 	}
@@ -5312,18 +5291,18 @@ set_config_option(const char *name, const char *value,
 								(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 						  errmsg("parameter \"%s\" requires a Boolean value",
 								 name)));
-						return false;
+						return 0;
 					}
 					if (!call_bool_check_hook(conf, &newval, &newextra,
 											  source, elevel))
-						return false;
+						return 0;
 				}
 				else if (source == PGC_S_DEFAULT)
 				{
 					newval = conf->boot_val;
 					if (!call_bool_check_hook(conf, &newval, &newextra,
 											  source, elevel))
-						return false;
+						return 0;
 				}
 				else
 				{
@@ -5335,11 +5314,14 @@ set_config_option(const char *name, const char *value,
 				if (prohibitValueChange)
 				{
 					if (*conf->variable != newval)
+					{
 						ereport(elevel,
 								(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
 								 errmsg("parameter \"%s\" cannot be changed without restarting the server",
 										name)));
-					return false;
+						return 0;
+					}
+					return -1;
 				}
 
 				if (changeVal)
@@ -5401,7 +5383,7 @@ set_config_option(const char *name, const char *value,
 						 errmsg("invalid value for parameter \"%s\": \"%s\"",
 								name, value),
 								 hintmsg ? errhint("%s", _(hintmsg)) : 0));
-						return false;
+						return 0;
 					}
 					if (newval < conf->min || newval > conf->max)
 					{
@@ -5409,18 +5391,18 @@ set_config_option(const char *name, const char *value,
 								(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 								 errmsg("%d is outside the valid range for parameter \"%s\" (%d .. %d)",
 										newval, name, conf->min, conf->max)));
-						return false;
+						return 0;
 					}
 					if (!call_int_check_hook(conf, &newval, &newextra,
 											 source, elevel))
-						return false;
+						return 0;
 				}
 				else if (source == PGC_S_DEFAULT)
 				{
 					newval = conf->boot_val;
 					if (!call_int_check_hook(conf, &newval, &newextra,
 											 source, elevel))
-						return false;
+						return 0;
 				}
 				else
 				{
@@ -5432,11 +5414,14 @@ set_config_option(const char *name, const char *value,
 				if (prohibitValueChange)
 				{
 					if (*conf->variable != newval)
+					{
 						ereport(elevel,
 								(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
 								 errmsg("parameter \"%s\" cannot be changed without restarting the server",
 										name)));
-					return false;
+						return 0;
+					}
+					return -1;
 				}
 
 				if (changeVal)
@@ -5495,7 +5480,7 @@ set_config_option(const char *name, const char *value,
 								(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 						  errmsg("parameter \"%s\" requires a numeric value",
 								 name)));
-						return false;
+						return 0;
 					}
 					if (newval < conf->min || newval > conf->max)
 					{
@@ -5503,18 +5488,18 @@ set_config_option(const char *name, const char *value,
 								(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 								 errmsg("%g is outside the valid range for parameter \"%s\" (%g .. %g)",
 										newval, name, conf->min, conf->max)));
-						return false;
+						return 0;
 					}
 					if (!call_real_check_hook(conf, &newval, &newextra,
 											  source, elevel))
-						return false;
+						return 0;
 				}
 				else if (source == PGC_S_DEFAULT)
 				{
 					newval = conf->boot_val;
 					if (!call_real_check_hook(conf, &newval, &newextra,
 											  source, elevel))
-						return false;
+						return 0;
 				}
 				else
 				{
@@ -5526,11 +5511,14 @@ set_config_option(const char *name, const char *value,
 				if (prohibitValueChange)
 				{
 					if (*conf->variable != newval)
+					{
 						ereport(elevel,
 								(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
 								 errmsg("parameter \"%s\" cannot be changed without restarting the server",
 										name)));
-					return false;
+						return 0;
+					}
+					return -1;
 				}
 
 				if (changeVal)
@@ -5589,7 +5577,7 @@ set_config_option(const char *name, const char *value,
 					 */
 					newval = guc_strdup(elevel, value);
 					if (newval == NULL)
-						return false;
+						return 0;
 
 					/*
 					 * The only built-in "parsing" check we have is to apply
@@ -5602,7 +5590,7 @@ set_config_option(const char *name, const char *value,
 												source, elevel))
 					{
 						free(newval);
-						return false;
+						return 0;
 					}
 				}
 				else if (source == PGC_S_DEFAULT)
@@ -5612,7 +5600,7 @@ set_config_option(const char *name, const char *value,
 					{
 						newval = guc_strdup(elevel, conf->boot_val);
 						if (newval == NULL)
-							return false;
+							return 0;
 					}
 					else
 						newval = NULL;
@@ -5621,7 +5609,7 @@ set_config_option(const char *name, const char *value,
 												source, elevel))
 					{
 						free(newval);
-						return false;
+						return 0;
 					}
 				}
 				else
@@ -5640,11 +5628,14 @@ set_config_option(const char *name, const char *value,
 					/* newval shouldn't be NULL, so we're a bit sloppy here */
 					if (*conf->variable == NULL || newval == NULL ||
 						strcmp(*conf->variable, newval) != 0)
+					{
 						ereport(elevel,
 								(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
 								 errmsg("parameter \"%s\" cannot be changed without restarting the server",
 										name)));
-					return false;
+						return 0;
+					}
+					return -1;
 				}
 
 				if (changeVal)
@@ -5718,18 +5709,18 @@ set_config_option(const char *name, const char *value,
 
 						if (hintmsg)
 							pfree(hintmsg);
-						return false;
+						return 0;
 					}
 					if (!call_enum_check_hook(conf, &newval, &newextra,
 											  source, elevel))
-						return false;
+						return 0;
 				}
 				else if (source == PGC_S_DEFAULT)
 				{
 					newval = conf->boot_val;
 					if (!call_enum_check_hook(conf, &newval, &newextra,
 											  source, elevel))
-						return false;
+						return 0;
 				}
 				else
 				{
@@ -5741,11 +5732,14 @@ set_config_option(const char *name, const char *value,
 				if (prohibitValueChange)
 				{
 					if (*conf->variable != newval)
+					{
 						ereport(elevel,
 								(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
 								 errmsg("parameter \"%s\" cannot be changed without restarting the server",
 										name)));
-					return false;
+						return 0;
+					}
+					return -1;
 				}
 
 				if (changeVal)
@@ -5794,7 +5788,7 @@ set_config_option(const char *name, const char *value,
 	if (changeVal && (record->flags & GUC_REPORT))
 		ReportGUCOption(record);
 
-	return true;
+	return changeVal ? 1 : -1;
 }
 
 
@@ -6095,12 +6089,12 @@ ExecSetVariableStmt(VariableSetStmt *stmt)
 	{
 		case VAR_SET_VALUE:
 		case VAR_SET_CURRENT:
-			set_config_option(stmt->name,
-							  ExtractSetVariableArgs(stmt),
-							  (superuser() ? PGC_SUSET : PGC_USERSET),
-							  PGC_S_SESSION,
-							  action,
-							  true);
+			(void) set_config_option(stmt->name,
+									 ExtractSetVariableArgs(stmt),
+									 (superuser() ? PGC_SUSET : PGC_USERSET),
+									 PGC_S_SESSION,
+									 action,
+									 true);
 			break;
 		case VAR_SET_MULTI:
 
@@ -6158,12 +6152,12 @@ ExecSetVariableStmt(VariableSetStmt *stmt)
 			break;
 		case VAR_SET_DEFAULT:
 		case VAR_RESET:
-			set_config_option(stmt->name,
-							  NULL,
-							  (superuser() ? PGC_SUSET : PGC_USERSET),
-							  PGC_S_SESSION,
-							  action,
-							  true);
+			(void) set_config_option(stmt->name,
+									 NULL,
+									 (superuser() ? PGC_SUSET : PGC_USERSET),
+									 PGC_S_SESSION,
+									 action,
+									 true);
 			break;
 		case VAR_RESET_ALL:
 			ResetAllOptions();
@@ -6203,12 +6197,12 @@ SetPGVariable(const char *name, List *args, bool is_local)
 	char	   *argstring = flatten_set_variable_args(name, args);
 
 	/* Note SET DEFAULT (argstring == NULL) is equivalent to RESET */
-	set_config_option(name,
-					  argstring,
-					  (superuser() ? PGC_SUSET : PGC_USERSET),
-					  PGC_S_SESSION,
-					  is_local ? GUC_ACTION_LOCAL : GUC_ACTION_SET,
-					  true);
+	(void) set_config_option(name,
+							 argstring,
+							 (superuser() ? PGC_SUSET : PGC_USERSET),
+							 PGC_S_SESSION,
+							 is_local ? GUC_ACTION_LOCAL : GUC_ACTION_SET,
+							 true);
 }
 
 /*
@@ -6246,12 +6240,12 @@ set_config_by_name(PG_FUNCTION_ARGS)
 		is_local = PG_GETARG_BOOL(2);
 
 	/* Note SET DEFAULT (argstring == NULL) is equivalent to RESET */
-	set_config_option(name,
-					  value,
-					  (superuser() ? PGC_SUSET : PGC_USERSET),
-					  PGC_S_SESSION,
-					  is_local ? GUC_ACTION_LOCAL : GUC_ACTION_SET,
-					  true);
+	(void) set_config_option(name,
+							 value,
+							 (superuser() ? PGC_SUSET : PGC_USERSET),
+							 PGC_S_SESSION,
+							 is_local ? GUC_ACTION_LOCAL : GUC_ACTION_SET,
+							 true);
 
 	/* get the new current value */
 	new_value = GetConfigOptionByName(name, NULL);
@@ -6421,7 +6415,7 @@ define_custom_variable(struct config_generic * variable)
 	{
 		if (set_config_option(name, value,
 							  phcontext, pHolder->gen.source,
-							  GUC_ACTION_SET, true))
+							  GUC_ACTION_SET, true) > 0)
 		{
 			/* Also copy over any saved source-location information */
 			if (pHolder->gen.sourcefile)
@@ -7943,9 +7937,9 @@ validate_option_array_item(const char *name, const char *value,
 	/* if a permissions error should be thrown, let set_config_option do it */
 
 	/* test for permissions and valid option value */
-	set_config_option(name, value,
-					  superuser() ? PGC_SUSET : PGC_USERSET,
-					  PGC_S_TEST, GUC_ACTION_SET, false);
+	(void) set_config_option(name, value,
+							 superuser() ? PGC_SUSET : PGC_USERSET,
+							 PGC_S_TEST, GUC_ACTION_SET, false);
 
 	return true;
 }

src/include/utils/guc.h

@@ -313,7 +313,7 @@ extern void ParseLongOption(const char *string, char **name, char **value);
 extern bool parse_int(const char *value, int *result, int flags,
 		  const char **hintmsg);
 extern bool parse_real(const char *value, double *result);
-extern bool set_config_option(const char *name, const char *value,
+extern int	set_config_option(const char *name, const char *value,
 				  GucContext context, GucSource source,
 				  GucAction action, bool changeVal);
 extern char *GetConfigOptionByName(const char *name, const char **varname);