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, 63 insertions, 89 deletions

diff --git a/src/backend/utils/misc/tzparser.c b/src/backend/utils/misc/tzparser.c
index e63ce58e69a..b2f6dd3a2bb 100644
--- a/src/backend/utils/misc/tzparser.c
+++ b/src/backend/utils/misc/tzparser.c
@@ -3,10 +3,12 @@
  * tzparser.c
  *	  Functions for parsing timezone offset files
  *
- * Note: we generally should not throw any errors in this file, but instead
- * try to return an error code.  This is not completely bulletproof at
- * present --- in particular out-of-memory will throw an error.  Could
- * probably fix with PG_TRY if necessary.
+ * Note: this code is invoked from the check_hook for the GUC variable
+ * timezone_abbreviations.  Therefore, it should report problems using
+ * GUC_check_errmsg() and related functions, and try to avoid throwing
+ * elog(ERROR).  This is not completely bulletproof at present --- in
+ * particular out-of-memory will throw an error.  Could probably fix with
+ * PG_TRY if necessary.
  *
  *
  * Portions Copyright (c) 1996-2011, PostgreSQL Global Development Group
@@ -24,15 +26,13 @@
 
 #include "miscadmin.h"
 #include "storage/fd.h"
-#include "utils/datetime.h"
+#include "utils/guc.h"
 #include "utils/memutils.h"
 #include "utils/tzparser.h"
 
 
 #define WHITESPACE " \t\n\r"
 
-static int	tz_elevel;			/* to avoid passing this around a lot */
-
 static bool validateTzEntry(tzEntry *tzentry);
 static bool splitTzLine(const char *filename, int lineno,
 			char *line, tzEntry *tzentry);
@@ -58,20 +58,16 @@ validateTzEntry(tzEntry *tzentry)
 	 */
 	if (strlen(tzentry->abbrev) > TOKMAXLEN)
 	{
-		ereport(tz_elevel,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("time zone abbreviation \"%s\" is too long (maximum %d characters) in time zone file \"%s\", line %d",
-						tzentry->abbrev, TOKMAXLEN,
-						tzentry->filename, tzentry->lineno)));
+		GUC_check_errmsg("time zone abbreviation \"%s\" is too long (maximum %d characters) in time zone file \"%s\", line %d",
+						 tzentry->abbrev, TOKMAXLEN,
+						 tzentry->filename, tzentry->lineno);
 		return false;
 	}
 	if (tzentry->offset % 900 != 0)
 	{
-		ereport(tz_elevel,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("time zone offset %d is not a multiple of 900 sec (15 min) in time zone file \"%s\", line %d",
-						tzentry->offset,
-						tzentry->filename, tzentry->lineno)));
+		GUC_check_errmsg("time zone offset %d is not a multiple of 900 sec (15 min) in time zone file \"%s\", line %d",
+						 tzentry->offset,
+						 tzentry->filename, tzentry->lineno);
 		return false;
 	}
 
@@ -81,11 +77,9 @@ validateTzEntry(tzEntry *tzentry)
 	if (tzentry->offset > 14 * 60 * 60 ||
 		tzentry->offset < -14 * 60 * 60)
 	{
-		ereport(tz_elevel,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("time zone offset %d is out of range in time zone file \"%s\", line %d",
-						tzentry->offset,
-						tzentry->filename, tzentry->lineno)));
+		GUC_check_errmsg("time zone offset %d is out of range in time zone file \"%s\", line %d",
+						 tzentry->offset,
+						 tzentry->filename, tzentry->lineno);
 		return false;
 	}
 
@@ -118,10 +112,8 @@ splitTzLine(const char *filename, int lineno, char *line, tzEntry *tzentry)
 	abbrev = strtok(line, WHITESPACE);
 	if (!abbrev)
 	{
-		ereport(tz_elevel,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("missing time zone abbreviation in time zone file \"%s\", line %d",
-						filename, lineno)));
+		GUC_check_errmsg("missing time zone abbreviation in time zone file \"%s\", line %d",
+						 filename, lineno);
 		return false;
 	}
 	tzentry->abbrev = abbrev;
@@ -129,19 +121,15 @@ splitTzLine(const char *filename, int lineno, char *line, tzEntry *tzentry)
 	offset = strtok(NULL, WHITESPACE);
 	if (!offset)
 	{
-		ereport(tz_elevel,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-		 errmsg("missing time zone offset in time zone file \"%s\", line %d",
-				filename, lineno)));
+		GUC_check_errmsg("missing time zone offset in time zone file \"%s\", line %d",
+						 filename, lineno);
 		return false;
 	}
 	tzentry->offset = strtol(offset, &offset_endptr, 10);
 	if (offset_endptr == offset || *offset_endptr != '\0')
 	{
-		ereport(tz_elevel,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("invalid number for time zone offset in time zone file \"%s\", line %d",
-						filename, lineno)));
+		GUC_check_errmsg("invalid number for time zone offset in time zone file \"%s\", line %d",
+						 filename, lineno);
 		return false;
 	}
 
@@ -163,10 +151,8 @@ splitTzLine(const char *filename, int lineno, char *line, tzEntry *tzentry)
 
 	if (remain[0] != '#')		/* must be a comment */
 	{
-		ereport(tz_elevel,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("invalid syntax in time zone file \"%s\", line %d",
-						filename, lineno)));
+		GUC_check_errmsg("invalid syntax in time zone file \"%s\", line %d",
+						 filename, lineno);
 		return false;
 	}
 	return true;
@@ -229,13 +215,11 @@ addToArray(tzEntry **base, int *arraysize, int n,
 				return n;
 			}
 			/* same abbrev but something is different, complain */
-			ereport(tz_elevel,
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				  errmsg("time zone abbreviation \"%s\" is multiply defined",
-						 entry->abbrev),
-					 errdetail("Entry in time zone file \"%s\", line %d, conflicts with entry in file \"%s\", line %d.",
-							   midptr->filename, midptr->lineno,
-							   entry->filename, entry->lineno)));
+			GUC_check_errmsg("time zone abbreviation \"%s\" is multiply defined",
+							 entry->abbrev);
+			GUC_check_errdetail("Entry in time zone file \"%s\", line %d, conflicts with entry in file \"%s\", line %d.",
+								midptr->filename, midptr->lineno,
+								entry->filename, entry->lineno);
 			return -1;
 		}
 	}
@@ -296,12 +280,10 @@ ParseTzFile(const char *filename, int depth,
 	{
 		if (!isalpha((unsigned char) *p))
 		{
-			/* at level 0, we need no ereport since guc.c will say enough */
+			/* at level 0, just use guc.c's regular "invalid value" message */
 			if (depth > 0)
-				ereport(tz_elevel,
-						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("invalid time zone file name \"%s\"",
-								filename)));
+				GUC_check_errmsg("invalid time zone file name \"%s\"",
+								 filename);
 			return -1;
 		}
 	}
@@ -313,10 +295,8 @@ ParseTzFile(const char *filename, int depth,
 	 */
 	if (depth > 3)
 	{
-		ereport(tz_elevel,
-				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-			 errmsg("time zone file recursion limit exceeded in file \"%s\"",
-					filename)));
+		GUC_check_errmsg("time zone file recursion limit exceeded in file \"%s\"",
+						 filename);
 		return -1;
 	}
 
@@ -340,12 +320,10 @@ ParseTzFile(const char *filename, int depth,
 		tzdir = AllocateDir(file_path);
 		if (tzdir == NULL)
 		{
-			ereport(tz_elevel,
-					(errcode_for_file_access(),
-					 errmsg("could not open directory \"%s\": %m",
-							file_path),
-					 errhint("This may indicate an incomplete PostgreSQL installation, or that the file \"%s\" has been moved away from its proper location.",
-							 my_exec_path)));
+			GUC_check_errmsg("could not open directory \"%s\": %m",
+							 file_path);
+			GUC_check_errhint("This may indicate an incomplete PostgreSQL installation, or that the file \"%s\" has been moved away from its proper location.",
+							  my_exec_path);
 			return -1;
 		}
 		FreeDir(tzdir);
@@ -356,10 +334,8 @@ ParseTzFile(const char *filename, int depth,
 		 * complaint is enough
 		 */
 		if (errno != ENOENT || depth > 0)
-			ereport(tz_elevel,
-					(errcode_for_file_access(),
-					 errmsg("could not read time zone file \"%s\": %m",
-							filename)));
+			GUC_check_errmsg("could not read time zone file \"%s\": %m",
+							 filename);
 
 		return -1;
 	}
@@ -371,10 +347,8 @@ ParseTzFile(const char *filename, int depth,
 		{
 			if (ferror(tzFile))
 			{
-				ereport(tz_elevel,
-						(errcode_for_file_access(),
-						 errmsg("could not read time zone file \"%s\": %m",
-								filename)));
+				GUC_check_errmsg("could not read time zone file \"%s\": %m",
+								 filename);
 				return -1;
 			}
 			/* else we're at EOF after all */
@@ -383,10 +357,8 @@ ParseTzFile(const char *filename, int depth,
 		if (strlen(tzbuf) == sizeof(tzbuf) - 1)
 		{
 			/* the line is too long for tzbuf */
-			ereport(tz_elevel,
-					(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
-				 errmsg("line is too long in time zone file \"%s\", line %d",
-						filename, lineno)));
+			GUC_check_errmsg("line is too long in time zone file \"%s\", line %d",
+							 filename, lineno);
 			return -1;
 		}
 
@@ -408,10 +380,8 @@ ParseTzFile(const char *filename, int depth,
 			includeFile = strtok(includeFile, WHITESPACE);
 			if (!includeFile || !*includeFile)
 			{
-				ereport(tz_elevel,
-						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("@INCLUDE without file name in time zone file \"%s\", line %d",
-								filename, lineno)));
+				GUC_check_errmsg("@INCLUDE without file name in time zone file \"%s\", line %d",
+								 filename, lineno);
 				return -1;
 			}
 			n = ParseTzFile(includeFile, depth + 1,
@@ -444,23 +414,20 @@ ParseTzFile(const char *filename, int depth,
 /*
  * load_tzoffsets --- read and parse the specified timezone offset file
  *
- * filename: name specified by user
- * doit: whether to actually apply the new values, or just check
- * elevel: elog reporting level (will be less than ERROR)
- *
- * Returns TRUE if OK, FALSE if not; should avoid erroring out
+ * On success, return a filled-in TimeZoneAbbrevTable, which must have been
+ * malloc'd not palloc'd.  On failure, return NULL, using GUC_check_errmsg
+ * and friends to give details of the problem.
  */
-bool
-load_tzoffsets(const char *filename, bool doit, int elevel)
+TimeZoneAbbrevTable *
+load_tzoffsets(const char *filename)
 {
+	TimeZoneAbbrevTable *result = NULL;
 	MemoryContext tmpContext;
 	MemoryContext oldContext;
 	tzEntry    *array;
 	int			arraysize;
 	int			n;
 
-	tz_elevel = elevel;
-
 	/*
 	 * Create a temp memory context to work in.  This makes it easy to clean
 	 * up afterwards.
@@ -479,13 +446,20 @@ load_tzoffsets(const char *filename, bool doit, int elevel)
 	/* Parse the file(s) */
 	n = ParseTzFile(filename, 0, &array, &arraysize, 0);
 
-	/* If no errors and we should apply the result, pass it to datetime.c */
-	if (n >= 0 && doit)
-		InstallTimeZoneAbbrevs(array, n);
+	/* If no errors so far, allocate result and let datetime.c convert data */
+	if (n >= 0)
+	{
+		result = malloc(offsetof(TimeZoneAbbrevTable, abbrevs) +
+						n * sizeof(datetkn));
+		if (!result)
+			GUC_check_errmsg("out of memory");
+		else
+			ConvertTimeZoneAbbrevs(result, array, n);
+	}
 
 	/* Clean up */
 	MemoryContextSwitchTo(oldContext);
 	MemoryContextDelete(tmpContext);
 
-	return (n >= 0);
+	return result;
 }