Improve consistency of error reporting in GUC assign_hook routines. Some

were reporting ERROR for interactive assignments and LOG for other cases,
some were saying nothing for non-interactive cases, and a few did yet other
things.  Make them use a new function GUC_complaint_elevel() to establish
a reasonably uniform policy about how to report.  There are still a few
edge cases such as assign_search_path(), but it's much better than before.
Per gripe from Devrim Gunduz and subsequent discussion.

As noted by Alvaro, it'd be better to fold these custom messages into the
standard "invalid parameter value" complaint from guc.c, perhaps as the DETAIL
field.  However that will require more redesign than seems prudent for 8.3.
This is a relatively safe, low-impact change that we can afford to risk now.

1 files changed, 46 insertions, 57 deletions

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index e4ea35ab53b..ec2c959af9c 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -9,7 +9,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/commands/variable.c,v 1.122 2007/11/15 21:14:34 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/commands/variable.c,v 1.123 2007/12/28 00:23:23 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -57,9 +57,8 @@ assign_datestyle(const char *value, bool doit, GucSource source)
 		/* syntax error in list */
 		pfree(rawstring);
 		list_free(elemlist);
-		if (source >= PGC_S_INTERACTIVE)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+		ereport(GUC_complaint_elevel(source),
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 				 errmsg("invalid list syntax for parameter \"datestyle\"")));
 		return NULL;
 	}
@@ -157,11 +156,10 @@ assign_datestyle(const char *value, bool doit, GucSource source)
 		}
 		else
 		{
-			if (source >= PGC_S_INTERACTIVE)
-				ereport(ERROR,
-						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("unrecognized \"datestyle\" key word: \"%s\"",
-								tok)));
+			ereport(GUC_complaint_elevel(source),
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("unrecognized \"datestyle\" key word: \"%s\"",
+							tok)));
 			ok = false;
 			break;
 		}
@@ -172,10 +170,9 @@ assign_datestyle(const char *value, bool doit, GucSource source)
 
 	if (!ok)
 	{
-		if (source >= PGC_S_INTERACTIVE)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					 errmsg("conflicting \"datestyle\" specifications")));
+		ereport(GUC_complaint_elevel(source),
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("conflicting \"datestyle\" specifications")));
 		return NULL;
 	}
 
@@ -271,9 +268,9 @@ assign_timezone(const char *value, bool doit, GucSource source)
 
 		/*
 		 * Try to parse it.  XXX an invalid interval format will result in
-		 * ereport, which is not desirable for GUC.  We did what we could to
-		 * guard against this in flatten_set_variable_args, but a string
-		 * coming in from postgresql.conf might contain anything.
+		 * ereport(ERROR), which is not desirable for GUC.  We did what we
+		 * could to guard against this in flatten_set_variable_args, but a
+		 * string coming in from postgresql.conf might contain anything.
 		 */
 		interval = DatumGetIntervalP(DirectFunctionCall3(interval_in,
 														 CStringGetDatum(val),
@@ -283,19 +280,17 @@ assign_timezone(const char *value, bool doit, GucSource source)
 		pfree(val);
 		if (interval->month != 0)
 		{
-			if (source >= PGC_S_INTERACTIVE)
-				ereport(ERROR,
-						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("invalid interval value for time zone: month not allowed")));
+			ereport(GUC_complaint_elevel(source),
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("invalid interval value for time zone: month not allowed")));
 			pfree(interval);
 			return NULL;
 		}
 		if (interval->day != 0)
 		{
-			if (source >= PGC_S_INTERACTIVE)
-				ereport(ERROR,
-						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("invalid interval value for time zone: day not allowed")));
+			ereport(GUC_complaint_elevel(source),
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("invalid interval value for time zone: day not allowed")));
 			pfree(interval);
 			return NULL;
 		}
@@ -361,7 +356,7 @@ assign_timezone(const char *value, bool doit, GucSource source)
 
 			if (!new_tz)
 			{
-				ereport((source >= PGC_S_INTERACTIVE) ? ERROR : LOG,
+				ereport(GUC_complaint_elevel(source),
 						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 						 errmsg("unrecognized time zone name: \"%s\"",
 								value)));
@@ -370,7 +365,7 @@ assign_timezone(const char *value, bool doit, GucSource source)
 
 			if (!tz_acceptable(new_tz))
 			{
-				ereport((source >= PGC_S_INTERACTIVE) ? ERROR : LOG,
+				ereport(GUC_complaint_elevel(source),
 						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 					   errmsg("time zone \"%s\" appears to use leap seconds",
 							  value),
@@ -493,7 +488,7 @@ assign_log_timezone(const char *value, bool doit, GucSource source)
 
 		if (!new_tz)
 		{
-			ereport((source >= PGC_S_INTERACTIVE) ? ERROR : LOG,
+			ereport(GUC_complaint_elevel(source),
 					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 					 errmsg("unrecognized time zone name: \"%s\"",
 							value)));
@@ -502,7 +497,7 @@ assign_log_timezone(const char *value, bool doit, GucSource source)
 
 		if (!tz_acceptable(new_tz))
 		{
-			ereport((source >= PGC_S_INTERACTIVE) ? ERROR : LOG,
+			ereport(GUC_complaint_elevel(source),
 					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 					 errmsg("time zone \"%s\" appears to use leap seconds",
 							value),
@@ -557,22 +552,20 @@ assign_XactIsoLevel(const char *value, bool doit, GucSource source)
 {
 	if (SerializableSnapshot != NULL)
 	{
-		if (source >= PGC_S_INTERACTIVE)
-			ereport(ERROR,
-					(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
-					 errmsg("SET TRANSACTION ISOLATION LEVEL must be called before any query")));
+		ereport(GUC_complaint_elevel(source),
+				(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
+				 errmsg("SET TRANSACTION ISOLATION LEVEL must be called before any query")));
 		/* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
-		else if (source != PGC_S_OVERRIDE)
+		if (source != PGC_S_OVERRIDE)
 			return NULL;
 	}
-	if (IsSubTransaction())
+	else if (IsSubTransaction())
 	{
-		if (source >= PGC_S_INTERACTIVE)
-			ereport(ERROR,
-					(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
-					 errmsg("SET TRANSACTION ISOLATION LEVEL must not be called in a subtransaction")));
+		ereport(GUC_complaint_elevel(source),
+				(errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
+				 errmsg("SET TRANSACTION ISOLATION LEVEL must not be called in a subtransaction")));
 		/* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
-		else if (source != PGC_S_OVERRIDE)
+		if (source != PGC_S_OVERRIDE)
 			return NULL;
 	}
 
@@ -667,11 +660,10 @@ assign_client_encoding(const char *value, bool doit, GucSource source)
 	 */
 	if (SetClientEncoding(encoding, doit) < 0)
 	{
-		if (source >= PGC_S_INTERACTIVE)
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("conversion between %s and %s is not supported",
-							value, GetDatabaseEncodingName())));
+		ereport(GUC_complaint_elevel(source),
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("conversion between %s and %s is not supported",
+						value, GetDatabaseEncodingName())));
 		return NULL;
 	}
 	return value;
@@ -740,10 +732,9 @@ assign_session_authorization(const char *value, bool doit, GucSource source)
 								 0, 0, 0);
 		if (!HeapTupleIsValid(roleTup))
 		{
-			if (source >= PGC_S_INTERACTIVE)
-				ereport(ERROR,
-						(errcode(ERRCODE_UNDEFINED_OBJECT),
-						 errmsg("role \"%s\" does not exist", value)));
+			ereport(GUC_complaint_elevel(source),
+					(errcode(ERRCODE_UNDEFINED_OBJECT),
+					 errmsg("role \"%s\" does not exist", value)));
 			return NULL;
 		}
 
@@ -853,10 +844,9 @@ assign_role(const char *value, bool doit, GucSource source)
 								 0, 0, 0);
 		if (!HeapTupleIsValid(roleTup))
 		{
-			if (source >= PGC_S_INTERACTIVE)
-				ereport(ERROR,
-						(errcode(ERRCODE_UNDEFINED_OBJECT),
-						 errmsg("role \"%s\" does not exist", value)));
+			ereport(GUC_complaint_elevel(source),
+					(errcode(ERRCODE_UNDEFINED_OBJECT),
+					 errmsg("role \"%s\" does not exist", value)));
 			return NULL;
 		}
 
@@ -870,11 +860,10 @@ assign_role(const char *value, bool doit, GucSource source)
 		 */
 		if (!is_member_of_role(GetSessionUserId(), roleid))
 		{
-			if (source >= PGC_S_INTERACTIVE)
-				ereport(ERROR,
-						(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-						 errmsg("permission denied to set role \"%s\"",
-								value)));
+			ereport(GUC_complaint_elevel(source),
+					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					 errmsg("permission denied to set role \"%s\"",
+							value)));
 			return NULL;
 		}
 	}