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".

6 files changed, 122 insertions, 34 deletions

diff --git a/src/include/utils/datetime.h b/src/include/utils/datetime.h
index 11672458f4..9911d20793 100644
--- a/src/include/utils/datetime.h
+++ b/src/include/utils/datetime.h
@@ -20,7 +20,9 @@
 #include <math.h>
 
 #include "utils/timestamp.h"
-#include "utils/tzparser.h"
+
+/* this struct is declared in utils/tzparser.h: */
+struct tzEntry;
 
 
 /* ----------------------------------------------------------------
@@ -203,6 +205,13 @@ typedef struct
 	char		value;			/* this may be unsigned, alas */
 } datetkn;
 
+/* one of its uses is in tables of time zone abbreviations */
+typedef struct TimeZoneAbbrevTable
+{
+	int			numabbrevs;
+	datetkn		abbrevs[1];		/* VARIABLE LENGTH ARRAY */
+} TimeZoneAbbrevTable;
+
 
 /* FMODULO()
  * Macro to replace modf(), which is broken on some platforms.
@@ -317,7 +326,10 @@ extern int	DecodeUnits(int field, char *lowtoken, int *val);
 extern int	j2day(int jd);
 
 extern bool CheckDateTokenTables(void);
-extern void InstallTimeZoneAbbrevs(tzEntry *abbrevs, int n);
+
+extern void ConvertTimeZoneAbbrevs(TimeZoneAbbrevTable *tbl,
+								   struct tzEntry *abbrevs, int n);
+extern void InstallTimeZoneAbbrevs(TimeZoneAbbrevTable *tbl);
 
 extern Datum pg_timezone_abbrevs(PG_FUNCTION_ARGS);
 extern Datum pg_timezone_names(PG_FUNCTION_ARGS);
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 71a1cdbd78..1f2a9f53c1 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -200,6 +200,15 @@ elog_finish(int elevel, const char *fmt,...)
 __attribute__((format(printf, 2, 3)));
 
 
+/* Support for constructing error strings separately from ereport() calls */
+
+extern void pre_format_elog_string(int errnumber, const char *domain);
+extern char *format_elog_string(const char *fmt,...)
+/* This extension allows gcc to check the format string for consistency with
+   the supplied arguments. */
+__attribute__((format(printf, 1, 2)));
+
+
 /* Support for attaching context information to error reports */
 
 typedef struct ErrorContextCallback
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index c07f263ea0..452310f2bd 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -97,7 +97,8 @@ typedef enum
 } GucSource;
 
 /*
- * Parsing the configuation file will return a list of name-value pairs
+ * Parsing the configuration file will return a list of name-value pairs
+ * with source location info.
  */
 typedef struct ConfigVariable
 {
@@ -117,7 +118,9 @@ extern bool ParseConfigFp(FILE *fp, const char *config_file,
 extern void FreeConfigVariables(ConfigVariable *list);
 
 /*
- * Enum values are made up of an array of name-value pairs
+ * The possible values of an enum variable are specified by an array of
+ * name-value pairs.  The "hidden" flag means the value is accepted but
+ * won't be displayed when guc.c is asked for a list of acceptable values.
  */
 struct config_enum_entry
 {
@@ -126,15 +129,26 @@ struct config_enum_entry
 	bool		hidden;
 };
 
-
-typedef const char *(*GucStringAssignHook) (const char *newval, bool doit, GucSource source);
-typedef bool (*GucBoolAssignHook) (bool newval, bool doit, GucSource source);
-typedef bool (*GucIntAssignHook) (int newval, bool doit, GucSource source);
-typedef bool (*GucRealAssignHook) (double newval, bool doit, GucSource source);
-typedef bool (*GucEnumAssignHook) (int newval, bool doit, GucSource source);
+/*
+ * Signatures for per-variable check/assign/show hook functions
+ */
+typedef bool (*GucBoolCheckHook) (bool *newval, void **extra, GucSource source);
+typedef bool (*GucIntCheckHook) (int *newval, void **extra, GucSource source);
+typedef bool (*GucRealCheckHook) (double *newval, void **extra, GucSource source);
+typedef bool (*GucStringCheckHook) (char **newval, void **extra, GucSource source);
+typedef bool (*GucEnumCheckHook) (int *newval, void **extra, GucSource source);
+
+typedef void (*GucBoolAssignHook) (bool newval, void *extra);
+typedef void (*GucIntAssignHook) (int newval, void *extra);
+typedef void (*GucRealAssignHook) (double newval, void *extra);
+typedef void (*GucStringAssignHook) (const char *newval, void *extra);
+typedef void (*GucEnumAssignHook) (int newval, void *extra);
 
 typedef const char *(*GucShowHook) (void);
 
+/*
+ * Miscellaneous
+ */
 typedef enum
 {
 	/* Types of set_config_option actions */
@@ -201,7 +215,6 @@ extern char *ConfigFileName;
 extern char *HbaFileName;
 extern char *IdentFileName;
 extern char *external_pid_file;
-extern char *XactIsoLevel_string;
 
 extern char *application_name;
 
@@ -209,6 +222,9 @@ extern int	tcp_keepalives_idle;
 extern int	tcp_keepalives_interval;
 extern int	tcp_keepalives_count;
 
+/*
+ * Functions exported by guc.c
+ */
 extern void SetConfigOption(const char *name, const char *value,
 				GucContext context, GucSource source);
 
@@ -220,6 +236,7 @@ extern void DefineCustomBoolVariable(
 						 bool bootValue,
 						 GucContext context,
 						 int flags,
+						 GucBoolCheckHook check_hook,
 						 GucBoolAssignHook assign_hook,
 						 GucShowHook show_hook);
 
@@ -233,6 +250,7 @@ extern void DefineCustomIntVariable(
 						int maxValue,
 						GucContext context,
 						int flags,
+						GucIntCheckHook check_hook,
 						GucIntAssignHook assign_hook,
 						GucShowHook show_hook);
 
@@ -246,6 +264,7 @@ extern void DefineCustomRealVariable(
 						 double maxValue,
 						 GucContext context,
 						 int flags,
+						 GucRealCheckHook check_hook,
 						 GucRealAssignHook assign_hook,
 						 GucShowHook show_hook);
 
@@ -257,6 +276,7 @@ extern void DefineCustomStringVariable(
 						   const char *bootValue,
 						   GucContext context,
 						   int flags,
+						   GucStringCheckHook check_hook,
 						   GucStringAssignHook assign_hook,
 						   GucShowHook show_hook);
 
@@ -269,6 +289,7 @@ extern void DefineCustomEnumVariable(
 						 const struct config_enum_entry * options,
 						 GucContext context,
 						 int flags,
+						 GucEnumCheckHook check_hook,
 						 GucEnumAssignHook assign_hook,
 						 GucShowHook show_hook);
 
@@ -308,8 +329,6 @@ extern ArrayType *GUCArrayAdd(ArrayType *array, const char *name, const char *va
 extern ArrayType *GUCArrayDelete(ArrayType *array, const char *name);
 extern ArrayType *GUCArrayReset(ArrayType *array);
 
-extern int	GUC_complaint_elevel(GucSource source);
-
 extern void pg_timezone_abbrev_initialize(void);
 
 #ifdef EXEC_BACKEND
@@ -317,6 +336,27 @@ extern void write_nondefault_variables(GucContext context);
 extern void read_nondefault_variables(void);
 #endif
 
+/* Support for messages reported from GUC check hooks */
+
+extern PGDLLIMPORT char *GUC_check_errmsg_string;
+extern PGDLLIMPORT char *GUC_check_errdetail_string;
+extern PGDLLIMPORT char *GUC_check_errhint_string;
+
+extern void GUC_check_errcode(int sqlerrcode);
+
+#define GUC_check_errmsg \
+	pre_format_elog_string(errno, TEXTDOMAIN), \
+	GUC_check_errmsg_string = format_elog_string
+
+#define GUC_check_errdetail \
+	pre_format_elog_string(errno, TEXTDOMAIN), \
+	GUC_check_errdetail_string = format_elog_string
+
+#define GUC_check_errhint \
+	pre_format_elog_string(errno, TEXTDOMAIN), \
+	GUC_check_errhint_string = format_elog_string
+
+
 /*
  * The following functions are not in guc.c, but are declared here to avoid
  * having to include guc.h in some widely used headers that it really doesn't
@@ -324,17 +364,16 @@ extern void read_nondefault_variables(void);
  */
 
 /* in commands/tablespace.c */
-extern const char *assign_default_tablespace(const char *newval,
-						  bool doit, GucSource source);
-extern const char *assign_temp_tablespaces(const char *newval,
-						bool doit, GucSource source);
+extern bool check_default_tablespace(char **newval, void **extra, GucSource source);
+extern bool check_temp_tablespaces(char **newval, void **extra, GucSource source);
+extern void assign_temp_tablespaces(const char *newval, void *extra);
 
 /* in catalog/namespace.c */
-extern const char *assign_search_path(const char *newval,
-				   bool doit, GucSource source);
+extern bool check_search_path(char **newval, void **extra, GucSource source);
+extern void assign_search_path(const char *newval, void *extra);
 
 /* in access/transam/xlog.c */
-extern bool assign_xlog_sync_method(int newval,
-						bool doit, GucSource source);
+extern bool check_wal_buffers(int *newval, void **extra, GucSource source);
+extern void assign_xlog_sync_method(int new_sync_method, void *extra);
 
 #endif   /* GUC_H */
diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index 073b77d28c..a1ca012ef9 100644
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -28,7 +28,7 @@ enum config_type
 	PGC_ENUM
 };
 
-union config_var_value
+union config_var_val
 {
 	bool		boolval;
 	int			intval;
@@ -38,6 +38,16 @@ union config_var_value
 };
 
 /*
+ * The actual value of a GUC variable can include a malloc'd opaque struct
+ * "extra", which is created by its check_hook and used by its assign_hook.
+ */
+typedef struct config_var_value
+{
+	union config_var_val val;
+	void	   *extra;
+} config_var_value;
+
+/*
  * Groupings to help organize all the run-time options for display
  */
 enum config_group
@@ -105,8 +115,8 @@ typedef struct guc_stack
 	int			nest_level;		/* nesting depth at which we made entry */
 	GucStackState state;		/* see enum above */
 	GucSource	source;			/* source of the prior value */
-	union config_var_value prior;		/* previous value of variable */
-	union config_var_value masked;		/* SET value in a GUC_SET_LOCAL entry */
+	config_var_value prior;		/* previous value of variable */
+	config_var_value masked;	/* SET value in a GUC_SET_LOCAL entry */
 	/* masked value's source must be PGC_S_SESSION, so no need to store it */
 } GucStack;
 
@@ -116,6 +126,11 @@ typedef struct guc_stack
  * The short description should be less than 80 chars in length. Some
  * applications may use the long description as well, and will append
  * it to the short description. (separated by a newline or '. ')
+ *
+ * Note that sourcefile/sourceline are kept here, and not pushed into stacked
+ * values, although in principle they belong with some stacked value if the
+ * active value is session- or transaction-local.  This is to avoid bloating
+ * stack entries.  We know they are only relevant when source == PGC_S_FILE.
  */
 struct config_generic
 {
@@ -132,7 +147,8 @@ struct config_generic
 	GucSource	reset_source;	/* source of the reset_value */
 	GucSource	source;			/* source of the current actual value */
 	GucStack   *stack;			/* stacked prior values */
-	char	   *sourcefile;		/* file this settings is from (NULL if not
+	void	   *extra;			/* "extra" pointer for current actual value */
+	char	   *sourcefile;		/* file current setting is from (NULL if not
 								 * file) */
 	int			sourceline;		/* line in source file */
 };
@@ -155,10 +171,12 @@ struct config_bool
 	/* constant fields, must be set correctly in initial value: */
 	bool	   *variable;
 	bool		boot_val;
+	GucBoolCheckHook check_hook;
 	GucBoolAssignHook assign_hook;
 	GucShowHook show_hook;
 	/* variable fields, initialized at runtime: */
 	bool		reset_val;
+	void	   *reset_extra;
 };
 
 struct config_int
@@ -169,10 +187,12 @@ struct config_int
 	int			boot_val;
 	int			min;
 	int			max;
+	GucIntCheckHook check_hook;
 	GucIntAssignHook assign_hook;
 	GucShowHook show_hook;
 	/* variable fields, initialized at runtime: */
 	int			reset_val;
+	void	   *reset_extra;
 };
 
 struct config_real
@@ -183,10 +203,12 @@ struct config_real
 	double		boot_val;
 	double		min;
 	double		max;
+	GucRealCheckHook check_hook;
 	GucRealAssignHook assign_hook;
 	GucShowHook show_hook;
 	/* variable fields, initialized at runtime: */
 	double		reset_val;
+	void	   *reset_extra;
 };
 
 struct config_string
@@ -195,10 +217,12 @@ struct config_string
 	/* constant fields, must be set correctly in initial value: */
 	char	  **variable;
 	const char *boot_val;
+	GucStringCheckHook check_hook;
 	GucStringAssignHook assign_hook;
 	GucShowHook show_hook;
 	/* variable fields, initialized at runtime: */
 	char	   *reset_val;
+	void	   *reset_extra;
 };
 
 struct config_enum
@@ -208,10 +232,12 @@ struct config_enum
 	int		   *variable;
 	int			boot_val;
 	const struct config_enum_entry *options;
+	GucEnumCheckHook check_hook;
 	GucEnumAssignHook assign_hook;
 	GucShowHook show_hook;
 	/* variable fields, initialized at runtime: */
 	int			reset_val;
+	void	   *reset_extra;
 };
 
 /* constant tables corresponding to enums above and in guc.h */
diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h
index 4c72fd0dc6..25b9d50915 100644
--- a/src/include/utils/pg_locale.h
+++ b/src/include/utils/pg_locale.h
@@ -33,14 +33,14 @@ extern char *localized_abbrev_months[];
 extern char *localized_full_months[];
 
 
-extern const char *locale_messages_assign(const char *value,
-					   bool doit, GucSource source);
-extern const char *locale_monetary_assign(const char *value,
-					   bool doit, GucSource source);
-extern const char *locale_numeric_assign(const char *value,
-					  bool doit, GucSource source);
-extern const char *locale_time_assign(const char *value,
-				   bool doit, GucSource source);
+extern bool check_locale_messages(char **newval, void **extra, GucSource source);
+extern void assign_locale_messages(const char *newval, void *extra);
+extern bool check_locale_monetary(char **newval, void **extra, GucSource source);
+extern void assign_locale_monetary(const char *newval, void *extra);
+extern bool check_locale_numeric(char **newval, void **extra, GucSource source);
+extern void assign_locale_numeric(const char *newval, void *extra);
+extern bool check_locale_time(char **newval, void **extra, GucSource source);
+extern void assign_locale_time(const char *newval, void *extra);
 
 extern bool check_locale(int category, const char *locale);
 extern char *pg_perm_setlocale(int category, const char *locale);
diff --git a/src/include/utils/tzparser.h b/src/include/utils/tzparser.h
index ba18819ef5..2065078116 100644
--- a/src/include/utils/tzparser.h
+++ b/src/include/utils/tzparser.h
@@ -13,6 +13,8 @@
 #ifndef TZPARSER_H
 #define TZPARSER_H
 
+#include "utils/datetime.h"
+
 /*
  * The result of parsing a timezone configuration file is an array of
  * these structs, in order by abbrev.  We export this because datetime.c
@@ -30,6 +32,6 @@ typedef struct tzEntry
 } tzEntry;
 
 
-extern bool load_tzoffsets(const char *filename, bool doit, int elevel);
+extern TimeZoneAbbrevTable *load_tzoffsets(const char *filename);
 
 #endif   /* TZPARSER_H */