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.

diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 419d26241782b5953ef87d0b57a466b761419baa..7c3fb95c73bcdd8aeb77b35511cddb34fb31039c 100644 (file)
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -923,11 +923,10 @@ assign_default_tablespace(const char *newval, bool doit, GucSource source)
                if (newval[0] != '\0' &&
                        !OidIsValid(get_tablespace_oid(newval)))
                {
-                       if (source >= PGC_S_INTERACTIVE)
-                               ereport(ERROR,
-                                               (errcode(ERRCODE_UNDEFINED_OBJECT),
-                                                errmsg("tablespace \"%s\" does not exist",
-                                                               newval)));
+                       ereport(GUC_complaint_elevel(source),
+                                       (errcode(ERRCODE_UNDEFINED_OBJECT),
+                                        errmsg("tablespace \"%s\" does not exist",
+                                                       newval)));
                        return NULL;
                }
        }

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index eb78be41cb53da456961720a9674ec527b5684e5..e856292e33f9eabfee91a5d4ef50e3bb73d6eb53 100644 (file)
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -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;
                }
        }

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 66538c166d40c44f47741232fdd2e26c9a48766f..73a1a6e5c016652a2db9cc03e0586edb9b77e802 100644 (file)
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2644,7 +2644,7 @@ assign_max_stack_depth(int newval, bool doit, GucSource source)
 
        if (stack_rlimit > 0 && newval_bytes > stack_rlimit - STACK_DEPTH_SLOP)
        {
-               ereport((source >= PGC_S_INTERACTIVE) ? ERROR : LOG,
+               ereport(GUC_complaint_elevel(source),
                                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
                                 errmsg("\"max_stack_depth\" must not exceed %ldkB",
                                                (stack_rlimit - STACK_DEPTH_SLOP) / 1024L),

diff --git a/src/backend/utils/misc/README b/src/backend/utils/misc/README
index e5f8545ea5bf9924006b720ac4046f77d3d71cb8..e99fd4b22d7a89fe100127faf41a32b5725cc272 100644 (file)
--- a/src/backend/utils/misc/README
+++ b/src/backend/utils/misc/README
@@ -28,16 +28,18 @@ function returns "true" then the assignment is completed; if it returns
 performed.  If "doit" is false then the function should simply check
 validity of newvalue and not change any derived state.  The "source" parameter
 indicates where the new value came from.  If it is >= PGC_S_INTERACTIVE,
-then we are performing an interactive assignment (e.g., a SET command).
-In such cases it is okay for the assign_hook to raise an error via ereport().
-If the function returns false for an interactive assignment then guc.c will
-report a generic "invalid value" error message.  (An internal ereport() in
-an assign_hook is only needed if you want to generate a specialized error
-message.)  But when source < PGC_S_INTERACTIVE, we are reading a
-non-interactive option source, such as postgresql.conf.  In this case the
-assign_hook should *not* ereport but should just return false if it doesn't
-like the newvalue.  (An ereport(LOG) call would be acceptable if you feel a
-need for a custom complaint in this situation.)
+then we are performing an interactive assignment (e.g., a SET command), and
+ereport(ERROR) is safe to do.  But when source < PGC_S_INTERACTIVE, we are
+reading a non-interactive option source, such as postgresql.conf.  In this
+case the assign_hook should *not* ereport but should just return false if it
+doesn't like the newvalue.
+
+If an assign_hook returns false then guc.c will report a generic "invalid
+value for option FOO" error message.  If you feel the need to provide a more
+specific error message, ereport() it using "GUC_complaint_elevel(source)"
+as the error level.  Note that this might return either ERROR or a lower level
+such as LOG, so the ereport call might or might not return.  If it does
+return, return false out of the assign_hook.
 
 For string variables, the signature for assign hooks is a bit different:
        const char *assign_hook(const char *newvalue,

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a777c8cb24bb11188392ff90765ad33a75f1e0df..1db87e6b7cfc152b39e1c46a8f02d8499bdac8b4 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4157,7 +4157,7 @@ call_string_assign_hook(GucStringAssignHook assign_hook,
  * If there is an error (non-existing option, invalid value) then an
  * ereport(ERROR) is thrown *unless* this is called in a context where we
  * don't want to ereport (currently, startup or SIGHUP config file reread).
- * In that case we write a suitable error message via ereport(DEBUG) and
+ * In that case we write a suitable error message via ereport(LOG) and
  * return false. This is working around the deficiencies in the ereport
  * mechanism, so don't blame me.  In all other cases, the function
  * returns true, including cases where the input is valid but we chose
@@ -4180,7 +4180,7 @@ set_config_option(const char *name, const char *value,
                 * To avoid cluttering the log, only the postmaster bleats loudly
                 * about problems with the config file.
                 */
-               elevel = IsUnderPostmaster ? DEBUG2 : LOG;
+               elevel = IsUnderPostmaster ? DEBUG3 : LOG;
        }
        else if (source == PGC_S_DATABASE || source == PGC_S_USER)
                elevel = INFO;
@@ -4803,6 +4803,41 @@ IsSuperuserConfigOption(const char *name)
 }
 
 
+/*
+ * GUC_complaint_elevel
+ *             Get the ereport error level to use in an assign_hook's error report.
+ *
+ * This should be used by assign hooks that want to emit a custom error
+ * report (in addition to the generic "invalid value for option FOO" that
+ * guc.c will provide).  Note that the result might be ERROR or a lower
+ * level, so the caller must be prepared for control to return from ereport,
+ * or not.  If control does return, return false/NULL from the hook function.
+ *
+ * At some point it'd be nice to replace this with a mechanism that allows
+ * the custom message to become the DETAIL line of guc.c's generic message.
+ */
+int
+GUC_complaint_elevel(GucSource source)
+{
+       int                     elevel;
+
+       if (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_INTERACTIVE)
+               elevel = LOG;
+       else
+               elevel = ERROR;
+
+       return elevel;
+}
+
+
 /*
  * flatten_set_variable_args
  *             Given a parsenode List as emitted by the grammar for SET,
@@ -6406,9 +6441,8 @@ assign_log_destination(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 \"log_destination\"")));
                return NULL;
        }
@@ -6431,9 +6465,8 @@ assign_log_destination(const char *value, bool doit, GucSource source)
 #endif
                else
                {
-                       if (source >= PGC_S_INTERACTIVE)
-                               ereport(ERROR,
-                                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                       ereport(GUC_complaint_elevel(source),
+                                       (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
                                  errmsg("unrecognized \"log_destination\" key word: \"%s\"",
                                                 tok)));
                        pfree(rawstring);
@@ -6719,10 +6752,9 @@ assign_phony_autocommit(bool newval, bool doit, GucSource source)
 {
        if (!newval)
        {
-               if (doit && source >= PGC_S_INTERACTIVE)
-                       ereport(ERROR,
-                                       (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                                        errmsg("SET AUTOCOMMIT TO OFF is no longer supported")));
+               ereport(GUC_complaint_elevel(source),
+                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                errmsg("SET AUTOCOMMIT TO OFF is no longer supported")));
                return false;
        }
        return true;
@@ -6791,9 +6823,12 @@ assign_debug_assertions(bool newval, bool doit, GucSource source)
 {
 #ifndef USE_ASSERT_CHECKING
        if (newval)
-               ereport(ERROR,
+       {
+               ereport(GUC_complaint_elevel(source),
                                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
                           errmsg("assertion checking is not supported by this build")));
+               return false;
+       }
 #endif
        return true;
 }
@@ -6803,9 +6838,12 @@ assign_ssl(bool newval, bool doit, GucSource source)
 {
 #ifndef USE_SSL
        if (newval)
-               ereport(ERROR,
+       {
+               ereport(GUC_complaint_elevel(source),
                                (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
                                 errmsg("SSL is not supported by this build")));
+               return false;
+       }
 #endif
        return true;
 }
@@ -6815,12 +6853,11 @@ assign_stage_log_stats(bool newval, bool doit, GucSource source)
 {
        if (newval && log_statement_stats)
        {
-               if (source >= PGC_S_INTERACTIVE)
-                       ereport(ERROR,
-                                       (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                                        errmsg("cannot enable parameter when \"log_statement_stats\" is true")));
+               ereport(GUC_complaint_elevel(source),
+                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                errmsg("cannot enable parameter when \"log_statement_stats\" is true")));
                /* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
-               else if (source != PGC_S_OVERRIDE)
+               if (source != PGC_S_OVERRIDE)
                        return false;
        }
        return true;
@@ -6832,14 +6869,13 @@ assign_log_stats(bool newval, bool doit, GucSource source)
        if (newval &&
                (log_parser_stats || log_planner_stats || log_executor_stats))
        {
-               if (source >= PGC_S_INTERACTIVE)
-                       ereport(ERROR,
-                                       (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                                        errmsg("cannot enable \"log_statement_stats\" when "
-                                                       "\"log_parser_stats\", \"log_planner_stats\", "
-                                                       "or \"log_executor_stats\" is true")));
+               ereport(GUC_complaint_elevel(source),
+                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                errmsg("cannot enable \"log_statement_stats\" when "
+                                               "\"log_parser_stats\", \"log_planner_stats\", "
+                                               "or \"log_executor_stats\" is true")));
                /* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
-               else if (source != PGC_S_OVERRIDE)
+               if (source != PGC_S_OVERRIDE)
                        return false;
        }
        return true;
@@ -6851,12 +6887,11 @@ assign_transaction_read_only(bool newval, bool doit, GucSource source)
        /* Can't go to r/w mode inside a r/o transaction */
        if (newval == false && XactReadOnly && IsSubTransaction())
        {
-               if (source >= PGC_S_INTERACTIVE)
-                       ereport(ERROR,
-                                       (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                                        errmsg("cannot set transaction read-write mode inside a read-only transaction")));
+               ereport(GUC_complaint_elevel(source),
+                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                errmsg("cannot set transaction read-write mode inside a read-only transaction")));
                /* source == PGC_S_OVERRIDE means do it anyway, eg at xact abort */
-               else if (source != PGC_S_OVERRIDE)
+               if (source != PGC_S_OVERRIDE)
                        return false;
        }
        return true;
@@ -6934,7 +6969,7 @@ assign_timezone_abbreviations(const char *newval, bool doit, GucSource source)
                 * and we use WARNING message level.
                 */
                if (source == PGC_S_FILE)
-                       elevel = IsUnderPostmaster ? DEBUG2 : LOG;
+                       elevel = IsUnderPostmaster ? DEBUG3 : LOG;
                else
                        elevel = WARNING;
                if (!load_tzoffsets(newval, doit, elevel))

diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index beb0decaf8b60936cfa7d02148006b582dd08ee0..81f4f9dcf1f710f038cc4b4d847e1dad2d2d3502 100644 (file)
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -221,6 +221,8 @@ extern void ProcessGUCArray(ArrayType *array,
 extern ArrayType *GUCArrayAdd(ArrayType *array, const char *name, const char *value);
 extern ArrayType *GUCArrayDelete(ArrayType *array, const char *name);
 
+extern int     GUC_complaint_elevel(GucSource source);
+
 extern void pg_timezone_abbrev_initialize(void);
 
 #ifdef EXEC_BACKEND