Improve define_custom_variable's handling of pre-existing settings.

Arrange for any problems with pre-existing settings to be reported as
WARNING not ERROR, so that we don't undesirably abort the loading of the
incoming add-on module.  The bad setting is just discarded, as though it
had never been applied at all.  (This requires a change in the API of
set_config_option.  After some thought I decided the most potentially
useful addition was to allow callers to just pass in a desired elevel.)

Arrange to restore the complete stacked state of the variable, rather than
cheesily reinstalling only the active value.  This ensures that custom GUCs
will behave unsurprisingly even when the module loading operation occurs
within nested subtransactions that have changed the active value.  Since a
module load could occur as a result of, eg, a PL function call, this is not
an unlikely scenario.

contrib/tsearch2/tsearch2.c

@@ -253,11 +253,8 @@ tsa_set_curcfg(PG_FUNCTION_ARGS)
 	name = DatumGetCString(DirectFunctionCall1(regconfigout,
 											   ObjectIdGetDatum(arg0)));
 
-	set_config_option("default_text_search_config", name,
-					  PGC_USERSET,
-					  PGC_S_SESSION,
-					  GUC_ACTION_SET,
-					  true);
+	SetConfigOption("default_text_search_config", name,
+					PGC_USERSET, PGC_S_SESSION);
 
 	PG_RETURN_VOID();
 }
@@ -271,11 +268,8 @@ tsa_set_curcfg_byname(PG_FUNCTION_ARGS)
 
 	name = text_to_cstring(arg0);
 
-	set_config_option("default_text_search_config", name,
-					  PGC_USERSET,
-					  PGC_S_SESSION,
-					  GUC_ACTION_SET,
-					  true);
+	SetConfigOption("default_text_search_config", name,
+					PGC_USERSET, PGC_S_SESSION);
 
 	PG_RETURN_VOID();
 }

src/backend/commands/extension.c

@@ -816,14 +816,14 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
 	if (client_min_messages < WARNING)
 		(void) set_config_option("client_min_messages", "warning",
 								 PGC_USERSET, PGC_S_SESSION,
-								 GUC_ACTION_LOCAL, true);
+								 GUC_ACTION_LOCAL, true, 0);
 
 	save_log_min_messages =
 		pstrdup(GetConfigOption("log_min_messages", false, false));
 	if (log_min_messages < WARNING)
 		(void) set_config_option("log_min_messages", "warning",
 								 PGC_SUSET, PGC_S_SESSION,
-								 GUC_ACTION_LOCAL, true);
+								 GUC_ACTION_LOCAL, true, 0);
 
 	/*
 	 * Set up the search path to contain the target schema, then the schemas
@@ -849,7 +849,7 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
 
 	(void) set_config_option("search_path", pathbuf.data,
 							 PGC_USERSET, PGC_S_SESSION,
-							 GUC_ACTION_LOCAL, true);
+							 GUC_ACTION_LOCAL, true, 0);
 
 	/*
 	 * Set creating_extension and related variables so that
@@ -915,13 +915,13 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
 	 */
 	(void) set_config_option("search_path", save_search_path,
 							 PGC_USERSET, PGC_S_SESSION,
-							 GUC_ACTION_LOCAL, true);
+							 GUC_ACTION_LOCAL, true, 0);
 	(void) set_config_option("client_min_messages", save_client_min_messages,
 							 PGC_USERSET, PGC_S_SESSION,
-							 GUC_ACTION_LOCAL, true);
+							 GUC_ACTION_LOCAL, true, 0);
 	(void) set_config_option("log_min_messages", save_log_min_messages,
 							 PGC_SUSET, PGC_S_SESSION,
-							 GUC_ACTION_LOCAL, true);
+							 GUC_ACTION_LOCAL, true, 0);
 }
 
 /*

src/backend/utils/adt/ri_triggers.c

@@ -2779,7 +2779,7 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
 	snprintf(workmembuf, sizeof(workmembuf), "%d", maintenance_work_mem);
 	(void) set_config_option("work_mem", workmembuf,
 							 PGC_USERSET, PGC_S_SESSION,
-							 GUC_ACTION_LOCAL, true);
+							 GUC_ACTION_LOCAL, true, 0);
 
 	if (SPI_connect() != SPI_OK_CONNECT)
 		elog(ERROR, "SPI_connect failed");
@@ -2868,7 +2868,7 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
 	snprintf(workmembuf, sizeof(workmembuf), "%d", old_work_mem);
 	(void) set_config_option("work_mem", workmembuf,
 							 PGC_USERSET, PGC_S_SESSION,
-							 GUC_ACTION_LOCAL, true);
+							 GUC_ACTION_LOCAL, true, 0);
 
 	return true;
 }

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

@@ -238,7 +238,7 @@ ProcessConfigFile(GucContext context)
 		/* Now we can re-apply the wired-in default (i.e., the boot_val) */
 		if (set_config_option(gconf->name, NULL,
 							  context, PGC_S_DEFAULT,
-							  GUC_ACTION_SET, true) > 0)
+							  GUC_ACTION_SET, true, 0) > 0)
 		{
 			/* Log the change if appropriate */
 			if (context == PGC_SIGHUP)
@@ -293,7 +293,7 @@ ProcessConfigFile(GucContext context)
 
 		scres = set_config_option(item->name, item->value,
 								  context, PGC_S_FILE,
-								  GUC_ACTION_SET, true);
+								  GUC_ACTION_SET, true, 0);
 		if (scres > 0)
 		{
 			/* variable was updated, so log the change if appropriate */

src/backend/utils/misc/guc.c

@@ -3260,6 +3260,11 @@ static void InitializeGUCOptionsFromEnvironment(void);
 static void InitializeOneGUCOption(struct config_generic * gconf);
 static void push_old_value(struct config_generic * gconf, GucAction action);
 static void ReportGUCOption(struct config_generic * record);
+static void reapply_stacked_values(struct config_generic * variable,
+					   struct config_string *pHolder,
+					   GucStack *stack,
+					   const char *curvalue,
+					   GucContext curscontext, GucSource cursource);
 static void ShowGUCConfigOption(const char *name, DestReceiver *dest);
 static void ShowAllGUCConfig(DestReceiver *dest);
 static char *_ShowOption(struct config_generic * record, bool use_units);
@@ -5020,6 +5025,10 @@ 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.
  *
+ * elevel should normally be passed as zero, allowing this function to make
+ * its standard choice of ereport level.  However some callers need to be
+ * able to override that choice; they should pass the ereport level to use.
+ *
  * Return value:
  *	+1: the value is valid and was successfully applied.
  *	0:  the name or value is invalid (but see below).
@@ -5028,34 +5037,37 @@ config_enum_get_options(struct config_enum * record, const char *prefix,
  * If there is an error (non-existing option, invalid value) then an
  * 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.
+ * and per-database or per-user settings, as well as callers who specify
+ * a less-than-ERROR elevel).  In those cases we write a suitable error
+ * message via ereport() and return 0.
  *
  * See also SetConfigOption for an external interface.
  */
 int
 set_config_option(const char *name, const char *value,
 				  GucContext context, GucSource source,
-				  GucAction action, bool changeVal)
+				  GucAction action, bool changeVal, int elevel)
 {
 	struct config_generic *record;
-	int			elevel;
 	bool		prohibitValueChange = false;
 	bool		makeDefault;
 
-	if (source == PGC_S_DEFAULT || source == PGC_S_FILE)
+	if (elevel == 0)
 	{
-		/*
-		 * To avoid cluttering the log, only the postmaster bleats loudly
-		 * about problems with the config file.
-		 */
-		elevel = IsUnderPostmaster ? DEBUG3 : LOG;
+		if (source == PGC_S_DEFAULT || source == PGC_S_FILE)
+		{
+			/*
+			 * To avoid cluttering the log, only the postmaster bleats loudly
+			 * about problems with the config file.
+			 */
+			elevel = IsUnderPostmaster ? DEBUG3 : LOG;
+		}
+		else if (source == PGC_S_DATABASE || source == PGC_S_USER ||
+				 source == PGC_S_DATABASE_USER)
+			elevel = WARNING;
+		else
+			elevel = ERROR;
 	}
-	else if (source == PGC_S_DATABASE || source == PGC_S_USER ||
-			 source == PGC_S_DATABASE_USER)
-		elevel = WARNING;
-	else
-		elevel = ERROR;
 
 	record = find_option(name, true, elevel);
 	if (record == NULL)
@@ -5798,9 +5810,11 @@ set_config_sourcefile(const char *name, char *sourcefile, int sourceline)
 }
 
 /*
- * Set a config option to the given value. See also set_config_option,
- * this is just the wrapper to be called from outside GUC.	NB: this
- * is used only for non-transactional operations.
+ * Set a config option to the given value.
+ *
+ * See also set_config_option; this is just the wrapper to be called from
+ * outside GUC.  (This function should be used when possible, because its API
+ * is more stable than set_config_option's.)
  *
  * Note: there is no support here for setting source file/line, as it
  * is currently not needed.
@@ -5810,7 +5824,7 @@ SetConfigOption(const char *name, const char *value,
 				GucContext context, GucSource source)
 {
 	(void) set_config_option(name, value, context, source,
-							 GUC_ACTION_SET, true);
+							 GUC_ACTION_SET, true, 0);
 }
 
 
@@ -6072,7 +6086,8 @@ ExecSetVariableStmt(VariableSetStmt *stmt)
 									 (superuser() ? PGC_SUSET : PGC_USERSET),
 									 PGC_S_SESSION,
 									 action,
-									 true);
+									 true,
+									 0);
 			break;
 		case VAR_SET_MULTI:
 
@@ -6135,7 +6150,8 @@ ExecSetVariableStmt(VariableSetStmt *stmt)
 									 (superuser() ? PGC_SUSET : PGC_USERSET),
 									 PGC_S_SESSION,
 									 action,
-									 true);
+									 true,
+									 0);
 			break;
 		case VAR_RESET_ALL:
 			ResetAllOptions();
@@ -6180,7 +6196,8 @@ SetPGVariable(const char *name, List *args, bool is_local)
 							 (superuser() ? PGC_SUSET : PGC_USERSET),
 							 PGC_S_SESSION,
 							 is_local ? GUC_ACTION_LOCAL : GUC_ACTION_SET,
-							 true);
+							 true,
+							 0);
 }
 
 /*
@@ -6223,7 +6240,8 @@ set_config_by_name(PG_FUNCTION_ARGS)
 							 (superuser() ? PGC_SUSET : PGC_USERSET),
 							 PGC_S_SESSION,
 							 is_local ? GUC_ACTION_LOCAL : GUC_ACTION_SET,
-							 true);
+							 true,
+							 0);
 
 	/* get the new current value */
 	new_value = GetConfigOptionByName(name, NULL);
@@ -6282,7 +6300,6 @@ 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;
 
@@ -6330,30 +6347,40 @@ define_custom_variable(struct config_generic * variable)
 	*res = variable;
 
 	/*
-	 * Assign the string value stored in the placeholder to the real variable.
+	 * Assign the string value(s) stored in the placeholder to the real
+	 * variable.  Essentially, we need to duplicate all the active and stacked
+	 * values, but with appropriate validation and datatype adjustment.
 	 *
-	 * XXX this is not really good enough --- it should be a nontransactional
-	 * assignment, since we don't want it to roll back if the current xact
-	 * fails later.  (Or do we?)
+	 * If an assignment fails, we report a WARNING and keep going.  We don't
+	 * want to throw ERROR for bad values, because it'd bollix the add-on
+	 * module that's presumably halfway through getting loaded.  In such cases
+	 * the default or previous state will become active instead.
 	 */
-	value = *pHolder->variable;
 
-	if (value)
-	{
-		if (set_config_option(name, value,
-							  pHolder->gen.scontext, pHolder->gen.source,
-							  GUC_ACTION_SET, true) != 0)
-		{
-			/* Also copy over any saved source-location information */
-			if (pHolder->gen.sourcefile)
-				set_config_sourcefile(name, pHolder->gen.sourcefile,
-									  pHolder->gen.sourceline);
-		}
-	}
+	/* First, apply the reset value if any */
+	if (pHolder->reset_val)
+		(void) set_config_option(name, pHolder->reset_val,
+								 pHolder->gen.reset_scontext,
+								 pHolder->gen.reset_source,
+								 GUC_ACTION_SET, true, WARNING);
+	/* That should not have resulted in stacking anything */
+	Assert(variable->stack == NULL);
+
+	/* Now, apply current and stacked values, in the order they were stacked */
+	reapply_stacked_values(variable, pHolder, pHolder->gen.stack,
+						   *(pHolder->variable),
+						   pHolder->gen.scontext, pHolder->gen.source);
+
+	/* Also copy over any saved source-location information */
+	if (pHolder->gen.sourcefile)
+		set_config_sourcefile(name, pHolder->gen.sourcefile,
+							  pHolder->gen.sourceline);
 
 	/*
-	 * Free up as much as we conveniently can of the placeholder structure
-	 * (this neglects any stack items...)
+	 * Free up as much as we conveniently can of the placeholder structure.
+	 * (This neglects any stack items, so it's possible for some memory to be
+	 * leaked.  Since this can only happen once per session per variable, it
+	 * doesn't seem worth spending much code on.)
 	 */
 	set_string_field(pHolder, pHolder->variable, NULL);
 	set_string_field(pHolder, &pHolder->reset_val, NULL);
@@ -6361,6 +6388,89 @@ define_custom_variable(struct config_generic * variable)
 	free(pHolder);
 }
 
+/*
+ * Recursive subroutine for define_custom_variable: reapply non-reset values
+ *
+ * We recurse so that the values are applied in the same order as originally.
+ * At each recursion level, apply the upper-level value (passed in) in the
+ * fashion implied by the stack entry.
+ */
+static void
+reapply_stacked_values(struct config_generic * variable,
+					   struct config_string *pHolder,
+					   GucStack *stack,
+					   const char *curvalue,
+					   GucContext curscontext, GucSource cursource)
+{
+	const char *name = variable->name;
+	GucStack   *oldvarstack = variable->stack;
+
+	if (stack != NULL)
+	{
+		/* First, recurse, so that stack items are processed bottom to top */
+		reapply_stacked_values(variable, pHolder, stack->prev,
+							   stack->prior.val.stringval,
+							   stack->scontext, stack->source);
+
+		/* See how to apply the passed-in value */
+		switch (stack->state)
+		{
+			case GUC_SAVE:
+				(void) set_config_option(name, curvalue,
+										 curscontext, cursource,
+										 GUC_ACTION_SAVE, true, WARNING);
+				break;
+
+			case GUC_SET:
+				(void) set_config_option(name, curvalue,
+										 curscontext, cursource,
+										 GUC_ACTION_SET, true, WARNING);
+				break;
+
+			case GUC_LOCAL:
+				(void) set_config_option(name, curvalue,
+										 curscontext, cursource,
+										 GUC_ACTION_LOCAL, true, WARNING);
+				break;
+
+			case GUC_SET_LOCAL:
+				/* first, apply the masked value as SET */
+				(void) set_config_option(name, stack->masked.val.stringval,
+										 stack->masked_scontext, PGC_S_SESSION,
+										 GUC_ACTION_SET, true, WARNING);
+				/* then apply the current value as LOCAL */
+				(void) set_config_option(name, curvalue,
+										 curscontext, cursource,
+										 GUC_ACTION_LOCAL, true, WARNING);
+				break;
+		}
+
+		/* If we successfully made a stack entry, adjust its nest level */
+		if (variable->stack != oldvarstack)
+			variable->stack->nest_level = stack->nest_level;
+	}
+	else
+	{
+		/*
+		 * We are at the end of the stack.  If the active/previous value is
+		 * different from the reset value, it must represent a previously
+		 * committed session value.  Apply it, and then drop the stack entry
+		 * that set_config_option will have created under the impression that
+		 * this is to be just a transactional assignment.  (We leak the stack
+		 * entry.)
+		 */
+		if (curvalue != pHolder->reset_val ||
+			curscontext != pHolder->gen.reset_scontext ||
+			cursource != pHolder->gen.reset_source)
+		{
+			(void) set_config_option(name, curvalue,
+									 curscontext, cursource,
+									 GUC_ACTION_SET, true, WARNING);
+			variable->stack = NULL;
+		}
+	}
+}
+
 void
 DefineCustomBoolVariable(const char *name,
 						 const char *short_desc,
@@ -7468,7 +7578,7 @@ read_nondefault_variables(void)
 
 		(void) set_config_option(varname, varvalue,
 								 varscontext, varsource,
-								 GUC_ACTION_SET, true);
+								 GUC_ACTION_SET, true, 0);
 		if (varsourcefile[0])
 			set_config_sourcefile(varname, varsourcefile, varsourceline);
 
@@ -7569,7 +7679,9 @@ ProcessGUCArray(ArrayType *array,
 			continue;
 		}
 
-		(void) set_config_option(name, value, context, source, action, true);
+		(void) set_config_option(name, value,
+								 context, source,
+								 action, true, 0);
 
 		free(name);
 		if (value)
@@ -7873,7 +7985,7 @@ validate_option_array_item(const char *name, const char *value,
 	/* test for permissions and valid option value */
 	(void) set_config_option(name, value,
 							 superuser() ? PGC_SUSET : PGC_USERSET,
-							 PGC_S_TEST, GUC_ACTION_SET, false);
+							 PGC_S_TEST, GUC_ACTION_SET, false, 0);
 
 	return true;
 }

src/include/utils/guc.h

@@ -315,7 +315,7 @@ extern bool parse_int(const char *value, int *result, int flags,
 extern bool parse_real(const char *value, double *result);
 extern int	set_config_option(const char *name, const char *value,
 				  GucContext context, GucSource source,
-				  GucAction action, bool changeVal);
+				  GucAction action, bool changeVal, int elevel);
 extern char *GetConfigOptionByName(const char *name, const char **varname);
 extern void GetConfigOptionByNum(int varnum, const char **values, bool *noshow);
 extern int	GetNumConfigOptions(void);