Revise the API for GUC variable assign hooks.

The previous functions of assign hooks are now split between check hooks
and assign hooks, where the former can fail but the latter shouldn't.
Aside from being conceptually clearer, this approach exposes the
"canonicalized" form of the variable value to guc.c without having to do
an actual assignment.  And that lets us fix the problem recently noted by
Bernd Helmle that the auto-tune patch for wal_buffers resulted in bogus
log messages about "parameter "wal_buffers" cannot be changed without
restarting the server".  There may be some speed advantage too, because
this design lets hook functions avoid re-parsing variable values when
restoring a previous state after a rollback (they can store a pre-parsed
representation of the value instead).  This patch also resolves a
longstanding annoyance about custom error messages from variable assign
hooks: they should modify, not appear separately from, guc.c's own message
about "invalid parameter value".

1 files changed, 445 insertions, 461 deletions

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 2a61ea3bc7c..2cec7130896 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -33,10 +33,10 @@
  */
 
 /*
- * assign_datestyle: GUC assign_hook for datestyle
+ * check_datestyle: GUC check_hook for datestyle
  */
-const char *
-assign_datestyle(const char *value, bool doit, GucSource source)
+bool
+check_datestyle(char **newval, void **extra, GucSource source)
 {
 	int			newDateStyle = DateStyle;
 	int			newDateOrder = DateOrder;
@@ -44,23 +44,22 @@ assign_datestyle(const char *value, bool doit, GucSource source)
 	bool		have_order = false;
 	bool		ok = true;
 	char	   *rawstring;
+	int		   *myextra;
 	char	   *result;
 	List	   *elemlist;
 	ListCell   *l;
 
 	/* Need a modifiable copy of string */
-	rawstring = pstrdup(value);
+	rawstring = pstrdup(*newval);
 
 	/* Parse string into list of identifiers */
 	if (!SplitIdentifierString(rawstring, ',', &elemlist))
 	{
 		/* syntax error in list */
+		GUC_check_errdetail("List syntax is invalid.");
 		pfree(rawstring);
 		list_free(elemlist);
-		ereport(GUC_complaint_elevel(source),
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("invalid list syntax for parameter \"datestyle\"")));
-		return NULL;
+		return false;
 	}
 
 	foreach(l, elemlist)
@@ -130,38 +129,38 @@ assign_datestyle(const char *value, bool doit, GucSource source)
 			 * Easiest way to get the current DEFAULT state is to fetch the
 			 * DEFAULT string from guc.c and recursively parse it.
 			 *
-			 * We can't simply "return assign_datestyle(...)" because we need
+			 * We can't simply "return check_datestyle(...)" because we need
 			 * to handle constructs like "DEFAULT, ISO".
 			 */
-			int			saveDateStyle = DateStyle;
-			int			saveDateOrder = DateOrder;
-			const char *subval;
+			char   *subval;
+			void   *subextra = NULL;
 
-			subval = assign_datestyle(GetConfigOptionResetString("datestyle"),
-									  true, source);
-			if (!have_style)
-				newDateStyle = DateStyle;
-			if (!have_order)
-				newDateOrder = DateOrder;
-			DateStyle = saveDateStyle;
-			DateOrder = saveDateOrder;
+			subval = strdup(GetConfigOptionResetString("datestyle"));
 			if (!subval)
 			{
 				ok = false;
 				break;
 			}
-			/* Here we know that our own return value is always malloc'd */
-			/* when doit is true */
-			free((char *) subval);
+			if (!check_datestyle(&subval, &subextra, source))
+			{
+				free(subval);
+				ok = false;
+				break;
+			}
+			myextra = (int *) subextra;
+			if (!have_style)
+				newDateStyle = myextra[0];
+			if (!have_order)
+				newDateOrder = myextra[1];
+			free(subval);
+			free(subextra);
 		}
 		else
 		{
-			ereport(GUC_complaint_elevel(source),
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					 errmsg("unrecognized \"datestyle\" key word: \"%s\"",
-							tok)));
-			ok = false;
-			break;
+			GUC_check_errdetail("Unrecognized key word: \"%s\".", tok);
+			pfree(rawstring);
+			list_free(elemlist);
+			return false;
 		}
 	}
 
@@ -170,24 +169,16 @@ assign_datestyle(const char *value, bool doit, GucSource source)
 
 	if (!ok)
 	{
-		ereport(GUC_complaint_elevel(source),
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("conflicting \"datestyle\" specifications")));
-		return NULL;
+		GUC_check_errdetail("Conflicting \"datestyle\" specifications.");
+		return false;
 	}
 
 	/*
-	 * If we aren't going to do the assignment, just return OK indicator.
-	 */
-	if (!doit)
-		return value;
-
-	/*
 	 * Prepare the canonical string to return.	GUC wants it malloc'd.
 	 */
 	result = (char *) malloc(32);
 	if (!result)
-		return NULL;
+		return false;
 
 	switch (newDateStyle)
 	{
@@ -217,14 +208,32 @@ assign_datestyle(const char *value, bool doit, GucSource source)
 			break;
 	}
 
+	free(*newval);
+	*newval = result;
+
 	/*
-	 * Finally, it's safe to assign to the global variables; the assignment
-	 * cannot fail now.
+	 * Set up the "extra" struct actually used by assign_datestyle.
 	 */
-	DateStyle = newDateStyle;
-	DateOrder = newDateOrder;
+	myextra = (int *) malloc(2 * sizeof(int));
+	if (!myextra)
+		return false;
+	myextra[0] = newDateStyle;
+	myextra[1] = newDateOrder;
+	*extra = (void *) myextra;
+
+	return true;
+}
+
+/*
+ * assign_datestyle: GUC assign_hook for datestyle
+ */
+void
+assign_datestyle(const char *newval, void *extra)
+{
+	int		   *myextra = (int *) extra;
 
-	return result;
+	DateStyle = myextra[0];
+	DateOrder = myextra[1];
 }
 
 
@@ -232,22 +241,58 @@ assign_datestyle(const char *value, bool doit, GucSource source)
  * TIMEZONE
  */
 
+typedef struct
+{
+	pg_tz	   *session_timezone;
+	int			CTimeZone;
+	bool		HasCTZSet;
+} timezone_extra;
+
 /*
- * assign_timezone: GUC assign_hook for timezone
+ * check_timezone: GUC check_hook for timezone
  */
-const char *
-assign_timezone(const char *value, bool doit, GucSource source)
+bool
+check_timezone(char **newval, void **extra, GucSource source)
 {
-	char	   *result;
+	timezone_extra myextra;
 	char	   *endptr;
 	double		hours;
 
+	if (*newval == NULL)
+	{
+		/*
+		 * The boot_val given for TimeZone in guc.c is NULL.  When we see this
+		 * we just do nothing.  If this isn't overridden from the config file
+		 * then pg_timezone_initialize() will eventually select a default
+		 * value from the environment.  This hack has two purposes: to avoid
+		 * wasting cycles loading values that might soon be overridden from
+		 * the config file, and to avoid trying to read the timezone files
+		 * during InitializeGUCOptions().  The latter doesn't work in an
+		 * EXEC_BACKEND subprocess because my_exec_path hasn't been set yet
+		 * and so we can't locate PGSHAREDIR.
+		 */
+		Assert(source == PGC_S_DEFAULT);
+		return true;
+	}
+
 	/*
-	 * Check for INTERVAL 'foo'
+	 * Initialize the "extra" struct that will be passed to assign_timezone.
+	 * We don't want to change any of the three global variables except as
+	 * specified by logic below.  To avoid leaking memory during failure
+	 * returns, we set up the struct contents in a local variable, and only
+	 * copy it to *extra at the end.
 	 */
-	if (pg_strncasecmp(value, "interval", 8) == 0)
+	myextra.session_timezone = session_timezone;
+	myextra.CTimeZone = CTimeZone;
+	myextra.HasCTZSet = HasCTZSet;
+
+	if (pg_strncasecmp(*newval, "interval", 8) == 0)
 	{
-		const char *valueptr = value;
+		/*
+		 * Support INTERVAL 'foo'.  This is for SQL spec compliance, not
+		 * because it has any actual real-world usefulness.
+		 */
+		const char *valueptr = *newval;
 		char	   *val;
 		Interval   *interval;
 
@@ -255,14 +300,14 @@ assign_timezone(const char *value, bool doit, GucSource source)
 		while (isspace((unsigned char) *valueptr))
 			valueptr++;
 		if (*valueptr++ != '\'')
-			return NULL;
+			return false;
 		val = pstrdup(valueptr);
 		/* Check and remove trailing quote */
 		endptr = strchr(val, '\'');
 		if (!endptr || endptr[1] != '\0')
 		{
 			pfree(val);
-			return NULL;
+			return false;
 		}
 		*endptr = '\0';
 
@@ -280,31 +325,25 @@ assign_timezone(const char *value, bool doit, GucSource source)
 		pfree(val);
 		if (interval->month != 0)
 		{
-			ereport(GUC_complaint_elevel(source),
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					 errmsg("invalid interval value for time zone: month not allowed")));
+			GUC_check_errdetail("Cannot specify months in time zone interval.");
 			pfree(interval);
-			return NULL;
+			return false;
 		}
 		if (interval->day != 0)
 		{
-			ereport(GUC_complaint_elevel(source),
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-			errmsg("invalid interval value for time zone: day not allowed")));
+			GUC_check_errdetail("Cannot specify days in time zone interval.");
 			pfree(interval);
-			return NULL;
+			return false;
 		}
-		if (doit)
-		{
-			/* Here we change from SQL to Unix sign convention */
+
+		/* Here we change from SQL to Unix sign convention */
 #ifdef HAVE_INT64_TIMESTAMP
-			CTimeZone = -(interval->time / USECS_PER_SEC);
+		myextra.CTimeZone = -(interval->time / USECS_PER_SEC);
 #else
-			CTimeZone = -interval->time;
+		myextra.CTimeZone = -interval->time;
 #endif
+		myextra.HasCTZSet = true;
 
-			HasCTZSet = true;
-		}
 		pfree(interval);
 	}
 	else
@@ -312,38 +351,12 @@ assign_timezone(const char *value, bool doit, GucSource source)
 		/*
 		 * Try it as a numeric number of hours (possibly fractional).
 		 */
-		hours = strtod(value, &endptr);
-		if (endptr != value && *endptr == '\0')
-		{
-			if (doit)
-			{
-				/* Here we change from SQL to Unix sign convention */
-				CTimeZone = -hours * SECS_PER_HOUR;
-				HasCTZSet = true;
-			}
-		}
-		else if (pg_strcasecmp(value, "UNKNOWN") == 0)
+		hours = strtod(*newval, &endptr);
+		if (endptr != *newval && *endptr == '\0')
 		{
-			/*
-			 * UNKNOWN is the value shown as the "default" for TimeZone in
-			 * guc.c.  We interpret it as being a complete no-op; we don't
-			 * change the timezone setting.  Note that if there is a known
-			 * timezone setting, we will return that name rather than UNKNOWN
-			 * as the canonical spelling.
-			 *
-			 * During GUC initialization, since the timezone library isn't set
-			 * up yet, pg_get_timezone_name will return NULL and we will leave
-			 * the setting as UNKNOWN.	If this isn't overridden from the
-			 * config file then pg_timezone_initialize() will eventually
-			 * select a default value from the environment.
-			 */
-			if (doit)
-			{
-				const char *curzone = pg_get_timezone_name(session_timezone);
-
-				if (curzone)
-					value = curzone;
-			}
+			/* Here we change from SQL to Unix sign convention */
+			myextra.CTimeZone = -hours * SECS_PER_HOUR;
+			myextra.HasCTZSet = true;
 		}
 		else
 		{
@@ -352,61 +365,83 @@ assign_timezone(const char *value, bool doit, GucSource source)
 			 */
 			pg_tz	   *new_tz;
 
-			new_tz = pg_tzset(value);
+			new_tz = pg_tzset(*newval);
 
 			if (!new_tz)
 			{
-				ereport(GUC_complaint_elevel(source),
-						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("unrecognized time zone name: \"%s\"",
-								value)));
-				return NULL;
+				/* Doesn't seem to be any great value in errdetail here */
+				return false;
 			}
 
 			if (!tz_acceptable(new_tz))
 			{
-				ereport(GUC_complaint_elevel(source),
-						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					   errmsg("time zone \"%s\" appears to use leap seconds",
-							  value),
-					errdetail("PostgreSQL does not support leap seconds.")));
-				return NULL;
+				GUC_check_errmsg("time zone \"%s\" appears to use leap seconds",
+								 *newval);
+				GUC_check_errdetail("PostgreSQL does not support leap seconds.");
+				return false;
 			}
 
-			if (doit)
-			{
-				/* Save the changed TZ */
-				session_timezone = new_tz;
-				HasCTZSet = false;
-			}
+			myextra.session_timezone = new_tz;
+			myextra.HasCTZSet = false;
 		}
 	}
 
 	/*
-	 * If we aren't going to do the assignment, just return OK indicator.
-	 */
-	if (!doit)
-		return value;
-
-	/*
 	 * Prepare the canonical string to return.	GUC wants it malloc'd.
+	 *
+	 * Note: the result string should be something that we'd accept as input.
+	 * We use the numeric format for interval cases, because it's simpler to
+	 * reload.  In the named-timezone case, *newval is already OK and need not
+	 * be changed; it might not have the canonical casing, but that's taken
+	 * care of by show_timezone.
 	 */
-	if (HasCTZSet)
+	if (myextra.HasCTZSet)
 	{
-		result = (char *) malloc(64);
+		char	*result = (char *) malloc(64);
+
 		if (!result)
-			return NULL;
+			return false;
 		snprintf(result, 64, "%.5f",
-				 (double) (-CTimeZone) / (double) SECS_PER_HOUR);
+				 (double) (-myextra.CTimeZone) / (double) SECS_PER_HOUR);
+		free(*newval);
+		*newval = result;
 	}
-	else
-		result = strdup(value);
 
-	return result;
+	/*
+	 * Pass back data for assign_timezone to use
+	 */
+	*extra = malloc(sizeof(timezone_extra));
+	if (!*extra)
+		return false;
+	memcpy(*extra, &myextra, sizeof(timezone_extra));
+
+	return true;
+}
+
+/*
+ * assign_timezone: GUC assign_hook for timezone
+ */
+void
+assign_timezone(const char *newval, void *extra)
+{
+	timezone_extra *myextra = (timezone_extra *) extra;
+
+	/* Do nothing for the boot_val default of NULL */
+	if (!myextra)
+		return;
+
+	session_timezone = myextra->session_timezone;
+	CTimeZone = myextra->CTimeZone;
+	HasCTZSet = myextra->HasCTZSet;
 }
 
 /*
  * show_timezone: GUC show_hook for timezone
+ *
+ * We wouldn't need this, except that historically interval values have been
+ * shown without an INTERVAL prefix, so the display format isn't what would
+ * be accepted as input.  Otherwise we could have check_timezone return the
+ * preferred string to begin with.
  */
 const char *
 show_timezone(void)
@@ -447,83 +482,66 @@ show_timezone(void)
  */
 
 /*
- * assign_log_timezone: GUC assign_hook for log_timezone
+ * check_log_timezone: GUC check_hook for log_timezone
  */
-const char *
-assign_log_timezone(const char *value, bool doit, GucSource source)
+bool
+check_log_timezone(char **newval, void **extra, GucSource source)
 {
-	char	   *result;
+	pg_tz	   *new_tz;
 
-	if (pg_strcasecmp(value, "UNKNOWN") == 0)
+	if (*newval == NULL)
 	{
 		/*
-		 * UNKNOWN is the value shown as the "default" for log_timezone in
-		 * guc.c.  We interpret it as being a complete no-op; we don't change
-		 * the timezone setting.  Note that if there is a known timezone
-		 * setting, we will return that name rather than UNKNOWN as the
-		 * canonical spelling.
-		 *
-		 * During GUC initialization, since the timezone library isn't set up
-		 * yet, pg_get_timezone_name will return NULL and we will leave the
-		 * setting as UNKNOWN.	If this isn't overridden from the config file
-		 * then pg_timezone_initialize() will eventually select a default
+		 * The boot_val given for log_timezone in guc.c is NULL.  When we see
+		 * this we just do nothing.  If this isn't overridden from the config
+		 * file then pg_timezone_initialize() will eventually select a default
 		 * value from the environment.
 		 */
-		if (doit)
-		{
-			const char *curzone = pg_get_timezone_name(log_timezone);
-
-			if (curzone)
-				value = curzone;
-		}
+		Assert(source == PGC_S_DEFAULT);
+		return true;
 	}
-	else
-	{
-		/*
-		 * Otherwise assume it is a timezone name, and try to load it.
-		 */
-		pg_tz	   *new_tz;
-
-		new_tz = pg_tzset(value);
 
-		if (!new_tz)
-		{
-			ereport(GUC_complaint_elevel(source),
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					 errmsg("unrecognized time zone name: \"%s\"",
-							value)));
-			return NULL;
-		}
+	/*
+	 * Otherwise assume it is a timezone name, and try to load it.
+	 */
+	new_tz = pg_tzset(*newval);
 
-		if (!tz_acceptable(new_tz))
-		{
-			ereport(GUC_complaint_elevel(source),
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					 errmsg("time zone \"%s\" appears to use leap seconds",
-							value),
-					 errdetail("PostgreSQL does not support leap seconds.")));
-			return NULL;
-		}
+	if (!new_tz)
+	{
+		/* Doesn't seem to be any great value in errdetail here */
+		return false;
+	}
 
-		if (doit)
-		{
-			/* Save the changed TZ */
-			log_timezone = new_tz;
-		}
+	if (!tz_acceptable(new_tz))
+	{
+		GUC_check_errmsg("time zone \"%s\" appears to use leap seconds",
+						 *newval);
+		GUC_check_errdetail("PostgreSQL does not support leap seconds.");
+		return false;
 	}
 
 	/*
-	 * If we aren't going to do the assignment, just return OK indicator.
+	 * Pass back data for assign_log_timezone to use
 	 */
-	if (!doit)
-		return value;
+	*extra = malloc(sizeof(pg_tz *));
+	if (!*extra)
+		return false;
+	memcpy(*extra, &new_tz, sizeof(pg_tz *));
 
-	/*
-	 * Prepare the canonical string to return.	GUC wants it malloc'd.
-	 */
-	result = strdup(value);
+	return true;
+}
+
+/*
+ * assign_log_timezone: GUC assign_hook for log_timezone
+ */
+void
+assign_log_timezone(const char *newval, void *extra)
+{
+	/* Do nothing for the boot_val default of NULL */
+	if (!extra)
+		return;
 
-	return result;
+	log_timezone = *((pg_tz **) extra);
 }
 
 /*
@@ -548,38 +566,33 @@ show_log_timezone(void)
  *
  * We allow idempotent changes (r/w -> r/w and r/o -> r/o) at any time, and
  * we also always allow changes from read-write to read-only.  However,
- * read-only to read-write may be changed only when source == PGC_S_OVERRIDE
- * (i.e. we're aborting a read only transaction and restoring the previous
- * setting) or in a top-level transaction that has not yet taken an initial
- * snapshot.
+ * read-only may be changed to read-write only when in a top-level transaction
+ * that has not yet taken an initial snapshot.  Can't do it in a hot standby
+ * slave, either.
  */
 bool
-assign_transaction_read_only(bool newval, bool doit, GucSource source)
+check_transaction_read_only(bool *newval, void **extra, GucSource source)
 {
-	if (source != PGC_S_OVERRIDE && newval == false && XactReadOnly)
+	if (*newval == false && XactReadOnly)
 	{
 		/* Can't go to r/w mode inside a r/o transaction */
 		if (IsSubTransaction())
 		{
-			ereport(GUC_complaint_elevel(source),
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					 errmsg("cannot set transaction read-write mode inside a read-only transaction")));
+			GUC_check_errcode(ERRCODE_ACTIVE_SQL_TRANSACTION);
+			GUC_check_errmsg("cannot set transaction read-write mode inside a read-only transaction");
 			return false;
 		}
 		/* Top level transaction can't change to r/w after first snapshot. */
 		if (FirstSnapshotSet)
 		{
-			ereport(GUC_complaint_elevel(source),
-					(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
-					 errmsg("transaction read-write mode must be set before any query")));
+			GUC_check_errcode(ERRCODE_ACTIVE_SQL_TRANSACTION);
+			GUC_check_errmsg("transaction read-write mode must be set before any query");
 			return false;
 		}
 		/* Can't go to r/w mode while recovery is still active */
 		if (RecoveryInProgress())
 		{
-			ereport(GUC_complaint_elevel(source),
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					 errmsg("cannot set transaction read-write mode during recovery")));
+			GUC_check_errmsg("cannot set transaction read-write mode during recovery");
 			return false;
 		}
 	}
@@ -591,76 +604,78 @@ assign_transaction_read_only(bool newval, bool doit, GucSource source)
  * SET TRANSACTION ISOLATION LEVEL
  *
  * We allow idempotent changes at any time, but otherwise this can only be
- * changed from a toplevel transaction that has not yet taken a snapshot, or
- * when source == PGC_S_OVERRIDE (i.e. we're aborting a transaction and
- * restoring the previously set value).
+ * changed in a toplevel transaction that has not yet taken a snapshot.
  */
-const char *
-assign_XactIsoLevel(const char *value, bool doit, GucSource source)
+bool
+check_XactIsoLevel(char **newval, void **extra, GucSource source)
 {
-	/* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
-	if (source != PGC_S_OVERRIDE && strcmp(value, XactIsoLevel_string) != 0)
+	int			newXactIsoLevel;
+
+	if (strcmp(*newval, "serializable") == 0)
+	{
+		newXactIsoLevel = XACT_SERIALIZABLE;
+	}
+	else if (strcmp(*newval, "repeatable read") == 0)
+	{
+		newXactIsoLevel = XACT_REPEATABLE_READ;
+	}
+	else if (strcmp(*newval, "read committed") == 0)
+	{
+		newXactIsoLevel = XACT_READ_COMMITTED;
+	}
+	else if (strcmp(*newval, "read uncommitted") == 0)
+	{
+		newXactIsoLevel = XACT_READ_UNCOMMITTED;
+	}
+	else if (strcmp(*newval, "default") == 0)
+	{
+		newXactIsoLevel = DefaultXactIsoLevel;
+	}
+	else
+		return false;
+
+	if (newXactIsoLevel != XactIsoLevel)
 	{
 		if (FirstSnapshotSet)
 		{
-			ereport(GUC_complaint_elevel(source),
-					(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
-					 errmsg("SET TRANSACTION ISOLATION LEVEL must be called before any query")));
-			return NULL;
+			GUC_check_errcode(ERRCODE_ACTIVE_SQL_TRANSACTION);
+			GUC_check_errmsg("SET TRANSACTION ISOLATION LEVEL must be called before any query");
+			return false;
 		}
 		/* We ignore a subtransaction setting it to the existing value. */
 		if (IsSubTransaction())
 		{
-			ereport(GUC_complaint_elevel(source),
-					(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
-					 errmsg("SET TRANSACTION ISOLATION LEVEL must not be called in a subtransaction")));
-			return NULL;
+			GUC_check_errcode(ERRCODE_ACTIVE_SQL_TRANSACTION);
+			GUC_check_errmsg("SET TRANSACTION ISOLATION LEVEL must not be called in a subtransaction");
+			return false;
 		}
 		/* Can't go to serializable mode while recovery is still active */
-		if (RecoveryInProgress() && strcmp(value, "serializable") == 0)
+		if (newXactIsoLevel == XACT_SERIALIZABLE && RecoveryInProgress())
 		{
-			ereport(GUC_complaint_elevel(source),
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					 errmsg("cannot use serializable mode in a hot standby"),
-					 errhint("You can use REPEATABLE READ instead.")));
+			GUC_check_errmsg("cannot use serializable mode in a hot standby");
+			GUC_check_errhint("You can use REPEATABLE READ instead.");
 			return false;
 		}
 	}
 
-	if (strcmp(value, "serializable") == 0)
-	{
-		if (doit)
-			XactIsoLevel = XACT_SERIALIZABLE;
-	}
-	else if (strcmp(value, "repeatable read") == 0)
-	{
-		if (doit)
-			XactIsoLevel = XACT_REPEATABLE_READ;
-	}
-	else if (strcmp(value, "read committed") == 0)
-	{
-		if (doit)
-			XactIsoLevel = XACT_READ_COMMITTED;
-	}
-	else if (strcmp(value, "read uncommitted") == 0)
-	{
-		if (doit)
-			XactIsoLevel = XACT_READ_UNCOMMITTED;
-	}
-	else if (strcmp(value, "default") == 0)
-	{
-		if (doit)
-			XactIsoLevel = DefaultXactIsoLevel;
-	}
-	else
-		return NULL;
+	*extra = malloc(sizeof(int));
+	if (!*extra)
+		return false;
+	*((int *) *extra) = newXactIsoLevel;
+
+	return true;
+}
 
-	return value;
+void
+assign_XactIsoLevel(const char *newval, void *extra)
+{
+	XactIsoLevel = *((int *) extra);
 }
 
 const char *
 show_XactIsoLevel(void)
 {
+	/* We need this because we don't want to show "default". */
 	switch (XactIsoLevel)
 	{
 		case XACT_READ_UNCOMMITTED:
@@ -681,25 +696,18 @@ show_XactIsoLevel(void)
  */
 
 bool
-assign_transaction_deferrable(bool newval, bool doit, GucSource source)
+check_transaction_deferrable(bool *newval, void **extra, GucSource source)
 {
-	/* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
-	if (source == PGC_S_OVERRIDE)
-		return true;
-
 	if (IsSubTransaction())
 	{
-		ereport(GUC_complaint_elevel(source),
-				(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
-				 errmsg("SET TRANSACTION [NOT] DEFERRABLE cannot be called within a subtransaction")));
+		GUC_check_errcode(ERRCODE_ACTIVE_SQL_TRANSACTION);
+		GUC_check_errmsg("SET TRANSACTION [NOT] DEFERRABLE cannot be called within a subtransaction");
 		return false;
 	}
-
 	if (FirstSnapshotSet)
 	{
-		ereport(GUC_complaint_elevel(source),
-				(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
-				 errmsg("SET TRANSACTION [NOT] DEFERRABLE must be called before any query")));
+		GUC_check_errcode(ERRCODE_ACTIVE_SQL_TRANSACTION);
+		GUC_check_errmsg("SET TRANSACTION [NOT] DEFERRABLE must be called before any query");
 		return false;
 	}
 
@@ -708,17 +716,34 @@ assign_transaction_deferrable(bool newval, bool doit, GucSource source)
 
 /*
  * Random number seed
+ *
+ * We can't roll back the random sequence on error, and we don't want
+ * config file reloads to affect it, so we only want interactive SET SEED
+ * commands to set it.  We use the "extra" storage to ensure that rollbacks
+ * don't try to do the operation again.
  */
 
 bool
-assign_random_seed(double value, bool doit, GucSource source)
+check_random_seed(double *newval, void **extra, GucSource source)
 {
-	/* Can't really roll back on error, so ignore non-interactive setting */
-	if (doit && source >= PGC_S_INTERACTIVE)
-		DirectFunctionCall1(setseed, Float8GetDatum(value));
+	*extra = malloc(sizeof(int));
+	if (!*extra)
+		return false;
+	/* Arm the assign only if source of value is an interactive SET */
+	*((int *) *extra) = (source >= PGC_S_INTERACTIVE);
+
 	return true;
 }
 
+void
+assign_random_seed(double newval, void *extra)
+{
+	/* We'll do this at most once for any setting of the GUC variable */
+	if (*((int *) extra))
+		DirectFunctionCall1(setseed, Float8GetDatum(newval));
+	*((int *) extra) = 0;
+}
+
 const char *
 show_random_seed(void)
 {
@@ -727,214 +752,189 @@ show_random_seed(void)
 
 
 /*
- * encoding handling functions
+ * SET CLIENT_ENCODING
  */
 
-const char *
-assign_client_encoding(const char *value, bool doit, GucSource source)
+bool
+check_client_encoding(char **newval, void **extra, GucSource source)
 {
 	int			encoding;
 
-	encoding = pg_valid_client_encoding(value);
+	/* Look up the encoding by name */
+	encoding = pg_valid_client_encoding(*newval);
 	if (encoding < 0)
-		return NULL;
+		return false;
 
 	/*
-	 * Note: if we are in startup phase then SetClientEncoding may not be able
-	 * to really set the encoding.	In this case we will assume that the
-	 * encoding is okay, and InitializeClientEncoding() will fix things once
-	 * initialization is complete.
+	 * If we are not within a transaction then PrepareClientEncoding will not
+	 * be able to look up the necessary conversion procs.  If we are still
+	 * starting up, it will return "OK" anyway, and InitializeClientEncoding
+	 * will fix things once initialization is far enough along.  After
+	 * startup, we'll fail.  This would only happen if someone tries to change
+	 * client_encoding in postgresql.conf and then SIGHUP existing sessions.
+	 * It seems like a bad idea for client_encoding to change that way anyhow,
+	 * so we don't go out of our way to support it.
+	 *
+	 * Note: in the postmaster, or any other process that never calls
+	 * InitializeClientEncoding, PrepareClientEncoding will always succeed,
+	 * and so will SetClientEncoding; but they won't do anything, which is OK.
 	 */
-	if (SetClientEncoding(encoding, doit) < 0)
+	if (PrepareClientEncoding(encoding) < 0)
 	{
-		ereport(GUC_complaint_elevel(source),
-				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-				 errmsg("conversion between %s and %s is not supported",
-						value, GetDatabaseEncodingName())));
-		return NULL;
+		if (IsTransactionState())
+		{
+			/* Must be a genuine no-such-conversion problem */
+			GUC_check_errcode(ERRCODE_FEATURE_NOT_SUPPORTED);
+			GUC_check_errdetail("Conversion between %s and %s is not supported.",
+								pg_encoding_to_char(encoding),
+								GetDatabaseEncodingName());
+		}
+		else
+		{
+			/* Provide a useful complaint */
+			GUC_check_errdetail("Cannot change \"client_encoding\" now.");
+		}
+		return false;
 	}
-	return value;
+
+	/*
+	 * Return the encoding's canonical name, and save its ID in *extra.
+	 */
+	free(*newval);
+	*newval = strdup(pg_encoding_to_char(encoding));
+	if (!*newval)
+		return false;
+
+	*extra = malloc(sizeof(int));
+	if (!*extra)
+		return false;
+	*((int *) *extra) = encoding;
+
+	return true;
+}
+
+void
+assign_client_encoding(const char *newval, void *extra)
+{
+	int			encoding = *((int *) extra);
+
+	/* We do not expect an error if PrepareClientEncoding succeeded */
+	if (SetClientEncoding(encoding) < 0)
+		elog(LOG, "SetClientEncoding(%d) failed", encoding);
 }
 
 
 /*
  * SET SESSION AUTHORIZATION
- *
- * When resetting session auth after an error, we can't expect to do catalog
- * lookups.  Hence, the stored form of the value must provide a numeric oid
- * that can be re-used directly.  We store the string in the form of
- * NAMEDATALEN 'x's, followed by T or F to indicate superuserness, followed
- * by the numeric oid, followed by a comma, followed by the role name.
- * This cannot be confused with a plain role name because of the NAMEDATALEN
- * limit on names, so we can tell whether we're being passed an initial
- * role name or a saved/restored value.  (NOTE: we rely on guc.c to have
- * properly truncated any incoming value, but not to truncate already-stored
- * values.	See GUC_IS_NAME processing.)
  */
-extern char *session_authorization_string;		/* in guc.c */
 
-const char *
-assign_session_authorization(const char *value, bool doit, GucSource source)
+typedef struct
 {
-	Oid			roleid = InvalidOid;
-	bool		is_superuser = false;
-	const char *actual_rolename = NULL;
-	char	   *result;
+	/* This is the "extra" state for both SESSION AUTHORIZATION and ROLE */
+	Oid			roleid;
+	bool		is_superuser;
+} role_auth_extra;
 
-	if (strspn(value, "x") == NAMEDATALEN &&
-		(value[NAMEDATALEN] == 'T' || value[NAMEDATALEN] == 'F'))
-	{
-		/* might be a saved userid string */
-		Oid			savedoid;
-		char	   *endptr;
+bool
+check_session_authorization(char **newval, void **extra, GucSource source)
+{
+	HeapTuple	roleTup;
+	Oid			roleid;
+	bool		is_superuser;
+	role_auth_extra *myextra;
 
-		savedoid = (Oid) strtoul(value + NAMEDATALEN + 1, &endptr, 10);
+	/* Do nothing for the boot_val default of NULL */
+	if (*newval == NULL)
+		return true;
 
-		if (endptr != value + NAMEDATALEN + 1 && *endptr == ',')
-		{
-			/* syntactically valid, so break out the data */
-			roleid = savedoid;
-			is_superuser = (value[NAMEDATALEN] == 'T');
-			actual_rolename = endptr + 1;
-		}
+	if (!IsTransactionState())
+	{
+		/*
+		 * Can't do catalog lookups, so fail.  The result of this is that
+		 * session_authorization cannot be set in postgresql.conf, which
+		 * seems like a good thing anyway, so we don't work hard to avoid it.
+		 */
+		return false;
 	}
 
-	if (roleid == InvalidOid)
+	/* Look up the username */
+	roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(*newval));
+	if (!HeapTupleIsValid(roleTup))
 	{
-		/* not a saved ID, so look it up */
-		HeapTuple	roleTup;
-
-		if (!IsTransactionState())
-		{
-			/*
-			 * Can't do catalog lookups, so fail.  The upshot of this is that
-			 * session_authorization cannot be set in postgresql.conf, which
-			 * seems like a good thing anyway.
-			 */
-			return NULL;
-		}
-
-		roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(value));
-		if (!HeapTupleIsValid(roleTup))
-		{
-			ereport(GUC_complaint_elevel(source),
-					(errcode(ERRCODE_UNDEFINED_OBJECT),
-					 errmsg("role \"%s\" does not exist", value)));
-			return NULL;
-		}
-
-		roleid = HeapTupleGetOid(roleTup);
-		is_superuser = ((Form_pg_authid) GETSTRUCT(roleTup))->rolsuper;
-		actual_rolename = value;
-
-		ReleaseSysCache(roleTup);
+		GUC_check_errmsg("role \"%s\" does not exist", *newval);
+		return false;
 	}
 
-	if (doit)
-		SetSessionAuthorization(roleid, is_superuser);
+	roleid = HeapTupleGetOid(roleTup);
+	is_superuser = ((Form_pg_authid) GETSTRUCT(roleTup))->rolsuper;
 
-	result = (char *) malloc(NAMEDATALEN + 32 + strlen(actual_rolename));
-	if (!result)
-		return NULL;
-
-	memset(result, 'x', NAMEDATALEN);
+	ReleaseSysCache(roleTup);
 
-	sprintf(result + NAMEDATALEN, "%c%u,%s",
-			is_superuser ? 'T' : 'F',
-			roleid,
-			actual_rolename);
+	/* Set up "extra" struct for assign_session_authorization to use */
+	myextra = (role_auth_extra *) malloc(sizeof(role_auth_extra));
+	if (!myextra)
+		return false;
+	myextra->roleid = roleid;
+	myextra->is_superuser = is_superuser;
+	*extra = (void *) myextra;
 
-	return result;
+	return true;
 }
 
-const char *
-show_session_authorization(void)
+void
+assign_session_authorization(const char *newval, void *extra)
 {
-	/*
-	 * Extract the user name from the stored string; see
-	 * assign_session_authorization
-	 */
-	const char *value = session_authorization_string;
-	Oid			savedoid;
-	char	   *endptr;
+	role_auth_extra *myextra = (role_auth_extra *) extra;
 
-	/* If session_authorization hasn't been set in this process, return "" */
-	if (value == NULL || value[0] == '\0')
-		return "";
+	/* Do nothing for the boot_val default of NULL */
+	if (!myextra)
+		return;
 
-	Assert(strspn(value, "x") == NAMEDATALEN &&
-		   (value[NAMEDATALEN] == 'T' || value[NAMEDATALEN] == 'F'));
-
-	savedoid = (Oid) strtoul(value + NAMEDATALEN + 1, &endptr, 10);
-
-	Assert(endptr != value + NAMEDATALEN + 1 && *endptr == ',');
-
-	return endptr + 1;
+	SetSessionAuthorization(myextra->roleid, myextra->is_superuser);
 }
 
 
 /*
  * SET ROLE
  *
- * When resetting session auth after an error, we can't expect to do catalog
- * lookups.  Hence, the stored form of the value must provide a numeric oid
- * that can be re-used directly.  We implement this exactly like SET
- * SESSION AUTHORIZATION.
- *
  * The SQL spec requires "SET ROLE NONE" to unset the role, so we hardwire
- * a translation of "none" to InvalidOid.
+ * a translation of "none" to InvalidOid.  Otherwise this is much like
+ * SET SESSION AUTHORIZATION.
  */
 extern char *role_string;		/* in guc.c */
 
-const char *
-assign_role(const char *value, bool doit, GucSource source)
+bool
+check_role(char **newval, void **extra, GucSource source)
 {
-	Oid			roleid = InvalidOid;
-	bool		is_superuser = false;
-	const char *actual_rolename = value;
-	char	   *result;
+	HeapTuple	roleTup;
+	Oid			roleid;
+	bool		is_superuser;
+	role_auth_extra *myextra;
 
-	if (strspn(value, "x") == NAMEDATALEN &&
-		(value[NAMEDATALEN] == 'T' || value[NAMEDATALEN] == 'F'))
+	if (strcmp(*newval, "none") == 0)
 	{
-		/* might be a saved userid string */
-		Oid			savedoid;
-		char	   *endptr;
-
-		savedoid = (Oid) strtoul(value + NAMEDATALEN + 1, &endptr, 10);
-
-		if (endptr != value + NAMEDATALEN + 1 && *endptr == ',')
-		{
-			/* syntactically valid, so break out the data */
-			roleid = savedoid;
-			is_superuser = (value[NAMEDATALEN] == 'T');
-			actual_rolename = endptr + 1;
-		}
+		/* hardwired translation */
+		roleid = InvalidOid;
+		is_superuser = false;
 	}
-
-	if (roleid == InvalidOid &&
-		strcmp(actual_rolename, "none") != 0)
+	else
 	{
-		/* not a saved ID, so look it up */
-		HeapTuple	roleTup;
-
 		if (!IsTransactionState())
 		{
 			/*
-			 * Can't do catalog lookups, so fail.  The upshot of this is that
+			 * Can't do catalog lookups, so fail.  The result of this is that
 			 * role cannot be set in postgresql.conf, which seems like a good
-			 * thing anyway.
+			 * thing anyway, so we don't work hard to avoid it.
 			 */
-			return NULL;
+			return false;
 		}
 
-		roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(value));
+		/* Look up the username */
+		roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(*newval));
 		if (!HeapTupleIsValid(roleTup))
 		{
-			ereport(GUC_complaint_elevel(source),
-					(errcode(ERRCODE_UNDEFINED_OBJECT),
-					 errmsg("role \"%s\" does not exist", value)));
-			return NULL;
+			GUC_check_errmsg("role \"%s\" does not exist", *newval);
+			return false;
 		}
 
 		roleid = HeapTupleGetOid(roleTup);
@@ -947,61 +947,45 @@ assign_role(const char *value, bool doit, GucSource source)
 		 */
 		if (!is_member_of_role(GetSessionUserId(), roleid))
 		{
-			ereport(GUC_complaint_elevel(source),
-					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-					 errmsg("permission denied to set role \"%s\"",
-							value)));
-			return NULL;
+			GUC_check_errcode(ERRCODE_INSUFFICIENT_PRIVILEGE);
+			GUC_check_errmsg("permission denied to set role \"%s\"",
+							 *newval);
+			return false;
 		}
 	}
 
-	if (doit)
-		SetCurrentRoleId(roleid, is_superuser);
-
-	result = (char *) malloc(NAMEDATALEN + 32 + strlen(actual_rolename));
-	if (!result)
-		return NULL;
+	/* Set up "extra" struct for assign_role to use */
+	myextra = (role_auth_extra *) malloc(sizeof(role_auth_extra));
+	if (!myextra)
+		return false;
+	myextra->roleid = roleid;
+	myextra->is_superuser = is_superuser;
+	*extra = (void *) myextra;
 
-	memset(result, 'x', NAMEDATALEN);
+	return true;
+}
 
-	sprintf(result + NAMEDATALEN, "%c%u,%s",
-			is_superuser ? 'T' : 'F',
-			roleid,
-			actual_rolename);
+void
+assign_role(const char *newval, void *extra)
+{
+	role_auth_extra *myextra = (role_auth_extra *) extra;
 
-	return result;
+	SetCurrentRoleId(myextra->roleid, myextra->is_superuser);
 }
 
 const char *
 show_role(void)
 {
 	/*
-	 * Extract the role name from the stored string; see assign_role
-	 */
-	const char *value = role_string;
-	Oid			savedoid;
-	char	   *endptr;
-
-	/* This special case only applies if no SET ROLE has been done */
-	if (value == NULL || strcmp(value, "none") == 0)
-		return "none";
-
-	Assert(strspn(value, "x") == NAMEDATALEN &&
-		   (value[NAMEDATALEN] == 'T' || value[NAMEDATALEN] == 'F'));
-
-	savedoid = (Oid) strtoul(value + NAMEDATALEN + 1, &endptr, 10);
-
-	Assert(endptr != value + NAMEDATALEN + 1 && *endptr == ',');
-
-	/*
-	 * Check that the stored string still matches the effective setting, else
-	 * return "none".  This is a kluge to deal with the fact that SET SESSION
-	 * AUTHORIZATION logically resets SET ROLE to NONE, but we cannot set the
-	 * GUC role variable from assign_session_authorization (because we haven't
-	 * got enough info to call set_config_option).
+	 * Check whether SET ROLE is active; if not return "none".  This is a
+	 * kluge to deal with the fact that SET SESSION AUTHORIZATION logically
+	 * resets SET ROLE to NONE, but we cannot set the GUC role variable from
+	 * assign_session_authorization (because we haven't got enough info to
+	 * call set_config_option).
 	 */
-	if (savedoid != GetCurrentRoleId())
+	if (!OidIsValid(GetCurrentRoleId()))
 		return "none";
 
-	return endptr + 1;
+	/* Otherwise we can just use the GUC string */
+	return role_string ? role_string : "none";
 }