Disallow setting client_min_messages > ERROR?
There's a thread on the ODBC list[1]/messages/by-id/EE586BE92A4AFB45B03310C2A0C0565D6D0EFC17@G01JPEXMBKW03 complaining about the fact that
it's possible to set client_min_messages to FATAL or PANIC, because
that makes ODBC misbehave. This is not terribly surprising, because
doing so arguably breaks the frontend protocol. The simple-query
section says this:
In the event of an error, ErrorResponse is issued followed by
ReadyForQuery.
and the extended-query section says this:
Therefore, an Execute phase is always terminated by the appearance of
exactly one of these messages: CommandComplete, EmptyQueryResponse (if
the portal was created from an empty query string), ErrorResponse, or
PortalSuspended.
and both of those are lies if an ERROR response gets suppressed thanks to
client_min_messages being set too high. It seems that libpq+psql manages
to survive the case (probably because psql is too stupid to notice that
anything is wrong), but I don't find it unreasonable that other clients
get hopelessly confused.
Hence, I propose that we should disallow setting client_min_messages
any higher than ERROR, and that this probably even amounts to a
back-patchable bug fix.
Thoughts?
regards, tom lane
[1]: /messages/by-id/EE586BE92A4AFB45B03310C2A0C0565D6D0EFC17@G01JPEXMBKW03
Hi,
On 2018-11-06 11:19:40 -0500, Tom Lane wrote:
Hence, I propose that we should disallow setting client_min_messages
any higher than ERROR, and that this probably even amounts to a
back-patchable bug fix.Thoughts?
Seems reasonable. I do think it's probably sensible to backpatch,
although I wonder if we shouldn't clamp the value to ERROR at log
emission error time, rather than via guc.c, so we don't prevent old code
/ postgresql.conf that set client_min_messages to > ERROR.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2018-11-06 11:19:40 -0500, Tom Lane wrote:
Hence, I propose that we should disallow setting client_min_messages
any higher than ERROR, and that this probably even amounts to a
back-patchable bug fix.Thoughts?
Seems reasonable. I do think it's probably sensible to backpatch,
although I wonder if we shouldn't clamp the value to ERROR at log
emission error time, rather than via guc.c, so we don't prevent old code
/ postgresql.conf that set client_min_messages to > ERROR.
Hm, do you really think there is any? And if there is, wouldn't we be
breaking it anyway thanks to the behavioral change?
regards, tom lane
On 2018-11-06 11:37:40 -0500, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2018-11-06 11:19:40 -0500, Tom Lane wrote:
Hence, I propose that we should disallow setting client_min_messages
any higher than ERROR, and that this probably even amounts to a
back-patchable bug fix.Thoughts?
Seems reasonable. I do think it's probably sensible to backpatch,
although I wonder if we shouldn't clamp the value to ERROR at log
emission error time, rather than via guc.c, so we don't prevent old code
/ postgresql.conf that set client_min_messages to > ERROR.Hm, do you really think there is any?
I'm not sure. But it sounds like it'd possibly slow adoption of the
minor releases if we said "hey, make sure that you nowhere set
client_min_messages > ERROR", even if it's not particularly meaningful
thing to do, as it'd still imply a fair bit of work for bigger
applications with not great standards.
And if there is, wouldn't we be breaking it anyway thanks to the
behavioral change?
Yea, possibly. I'd assume that it'd mostly have been set out of a
mistake, and never really noticed, because it's hard to notice the
consequences when things are ok.
Greetings,
Andres Freund
On 2018-Nov-06, Andres Freund wrote:
On 2018-11-06 11:37:40 -0500, Tom Lane wrote:
Hm, do you really think there is any?
I'm not sure. But it sounds like it'd possibly slow adoption of the
minor releases if we said "hey, make sure that you nowhere set
client_min_messages > ERROR", even if it's not particularly meaningful
thing to do, as it'd still imply a fair bit of work for bigger
applications with not great standards.
I agree -- it seems better to have a benign no-op and prevent this kind
of silly rationale from preventing upgrades.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Two options presented:
- Hard patch removes FATAL/PANIC from client_message_level_options in
guc.c, which also seems to make sense in regard to it's double-usage
with trace_recovery_messages.
- Soft patch keeps FATAL/PANIC in client_message_level_options but coerces
client_min_messages to ERROR when set to FATAL/PANIC and issues a warning.
This also exports error_severity from elog.c to retrieve severity -> text
mappings for the warning message.
--
Jonah H. Harris
Attachments:
client_min_messages_config_hard.patchapplication/octet-stream; name=client_min_messages_config_hard.patchDownload
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index e471d7f53c..1679cda8cd 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -229,8 +229,6 @@ static const struct config_enum_entry client_message_level_options[] = {
{"notice", NOTICE, false},
{"warning", WARNING, false},
{"error", ERROR, false},
- {"fatal", FATAL, true},
- {"panic", PANIC, true},
{NULL, 0, false}
};
client_min_messages_config_soft.patchapplication/octet-stream; name=client_min_messages_config_soft.patchDownload
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index b9c11301ca..9bf86b57a0 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -177,7 +177,6 @@ static void write_csvlog(ErrorData *edata);
static void send_message_to_server_log(ErrorData *edata);
static void write_pipe_chunks(char *data, int len, int dest);
static void send_message_to_frontend(ErrorData *edata);
-static const char *error_severity(int elevel);
static void append_with_tabs(StringInfo buf, const char *str);
static bool is_log_level_output(int elevel, int log_min_level);
@@ -3325,7 +3324,7 @@ send_message_to_frontend(ErrorData *edata)
* The string is not localized here, but we mark the strings for translation
* so that callers can invoke _() on the result.
*/
-static const char *
+const char *
error_severity(int elevel)
{
const char *prefix;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index e471d7f53c..004a0497e7 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -166,6 +166,7 @@ static int syslog_facility = 0;
static void assign_syslog_facility(int newval, void *extra);
static void assign_syslog_ident(const char *newval, void *extra);
static void assign_session_replication_role(int newval, void *extra);
+static bool check_client_min_messages (int *newval, void **extra, GucSource source);
static bool check_temp_buffers(int *newval, void **extra, GucSource source);
static bool check_bonjour(bool *newval, void **extra, GucSource source);
static bool check_ssl(bool *newval, void **extra, GucSource source);
@@ -3931,7 +3932,7 @@ static struct config_enum ConfigureNamesEnum[] =
},
&client_min_messages,
NOTICE, client_message_level_options,
- NULL, NULL, NULL
+ check_client_min_messages, NULL, NULL
},
{
@@ -10259,6 +10260,22 @@ call_enum_check_hook(struct config_enum *conf, int *newval, void **extra,
return true;
}
+static bool
+check_client_min_messages (int *newval, void **extra, GucSource source)
+{
+ int newLogLevel = *newval;
+
+ if (newLogLevel == FATAL || newLogLevel == PANIC)
+ {
+ elog(WARNING,
+ "frontend protocol requires %s or above (coerced to %s from %s)",
+ error_severity(ERROR), error_severity(ERROR),
+ error_severity(newLogLevel));
+ *newval = ERROR;
+ }
+
+ return true;
+}
/*
* check_hook, assign_hook and show_hook subroutines
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 33c6b53e27..2e3f9e2163 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -416,6 +416,7 @@ extern bool syslog_split_messages;
extern void DebugFileOpen(void);
extern char *unpack_sql_state(int sql_state);
extern bool in_error_recursion_trouble(void);
+extern const char *error_severity(int elevel);
#ifdef HAVE_SYSLOG
extern void set_syslog_parameters(const char *ident, int facility);
Import Notes
Resolved by subject fallback
On Tue, 6 Nov 2018 at 14:07, Jonah H. Harris <jonah.harris@gmail.com> wrote:
Two options presented:
- Hard patch removes FATAL/PANIC from client_message_level_options in
guc.c, which also seems to make sense in regard to it's double-usage
with trace_recovery_messages.- Soft patch keeps FATAL/PANIC in client_message_level_options but coerces
client_min_messages to ERROR when set to FATAL/PANIC and issues a warning.
This also exports error_severity from elog.c to retrieve severity -> text
mappings for the warning message.
What about no-op (soft patch) for 11.1 and backpatches, error (hard patch)
for 12?
On Tue, Nov 6, 2018 at 2:46 PM Isaac Morland <isaac.morland@gmail.com>
wrote:
On Tue, 6 Nov 2018 at 14:07, Jonah H. Harris <jonah.harris@gmail.com>
wrote:Two options presented:
- Hard patch removes FATAL/PANIC from client_message_level_options in
guc.c, which also seems to make sense in regard to it's double-usage
with trace_recovery_messages.- Soft patch keeps FATAL/PANIC in client_message_level_options but
coerces client_min_messages to ERROR when set to FATAL/PANIC and issues a
warning. This also exports error_severity from elog.c to retrieve severity
-> text mappings for the warning message.What about no-op (soft patch) for 11.1 and backpatches, error (hard patch)
for 12?
I'm usually a fan of the hard fix... but I do see the point they've made
about during an upgrade.
Also, fixed wording in the soft patch (frontend protocol requires %s or
above -> frontend protocol requires %s or below) attached.
--
Jonah H. Harris
Attachments:
client_min_messages_config_soft-v2.patchapplication/octet-stream; name=client_min_messages_config_soft-v2.patchDownload
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index b9c11301ca..9bf86b57a0 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -177,7 +177,6 @@ static void write_csvlog(ErrorData *edata);
static void send_message_to_server_log(ErrorData *edata);
static void write_pipe_chunks(char *data, int len, int dest);
static void send_message_to_frontend(ErrorData *edata);
-static const char *error_severity(int elevel);
static void append_with_tabs(StringInfo buf, const char *str);
static bool is_log_level_output(int elevel, int log_min_level);
@@ -3325,7 +3324,7 @@ send_message_to_frontend(ErrorData *edata)
* The string is not localized here, but we mark the strings for translation
* so that callers can invoke _() on the result.
*/
-static const char *
+const char *
error_severity(int elevel)
{
const char *prefix;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index e471d7f53c..004a0497e7 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -166,6 +166,7 @@ static int syslog_facility = 0;
static void assign_syslog_facility(int newval, void *extra);
static void assign_syslog_ident(const char *newval, void *extra);
static void assign_session_replication_role(int newval, void *extra);
+static bool check_client_min_messages (int *newval, void **extra, GucSource source);
static bool check_temp_buffers(int *newval, void **extra, GucSource source);
static bool check_bonjour(bool *newval, void **extra, GucSource source);
static bool check_ssl(bool *newval, void **extra, GucSource source);
@@ -3931,7 +3932,7 @@ static struct config_enum ConfigureNamesEnum[] =
},
&client_min_messages,
NOTICE, client_message_level_options,
- NULL, NULL, NULL
+ check_client_min_messages, NULL, NULL
},
{
@@ -10259,6 +10260,22 @@ call_enum_check_hook(struct config_enum *conf, int *newval, void **extra,
return true;
}
+static bool
+check_client_min_messages (int *newval, void **extra, GucSource source)
+{
+ int newLogLevel = *newval;
+
+ if (newLogLevel == FATAL || newLogLevel == PANIC)
+ {
+ elog(WARNING,
+ "frontend protocol requires %s or below (coerced to %s from %s)",
+ error_severity(ERROR), error_severity(ERROR),
+ error_severity(newLogLevel));
+ *newval = ERROR;
+ }
+
+ return true;
+}
/*
* check_hook, assign_hook and show_hook subroutines
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 33c6b53e27..2e3f9e2163 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -416,6 +416,7 @@ extern bool syslog_split_messages;
extern void DebugFileOpen(void);
extern char *unpack_sql_state(int sql_state);
extern bool in_error_recursion_trouble(void);
+extern const char *error_severity(int elevel);
#ifdef HAVE_SYSLOG
extern void set_syslog_parameters(const char *ident, int facility);
On Tue, Nov 6, 2018 at 11:48 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
I agree -- it seems better to have a benign no-op and prevent this kind
of silly rationale from preventing upgrades.
+1.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Andres Freund <andres@anarazel.de> writes:
On 2018-11-06 11:37:40 -0500, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
Seems reasonable. I do think it's probably sensible to backpatch,
although I wonder if we shouldn't clamp the value to ERROR at log
emission error time, rather than via guc.c, so we don't prevent old code
/ postgresql.conf that set client_min_messages to > ERROR.
Hm, do you really think there is any?
I'm not sure. But it sounds like it'd possibly slow adoption of the
minor releases if we said "hey, make sure that you nowhere set
client_min_messages > ERROR", even if it's not particularly meaningful
thing to do, as it'd still imply a fair bit of work for bigger
applications with not great standards.
OK, so the consensus seems to be that the back branches should continue
to allow you to set client_min_messages = FATAL/PANIC, but then ignore
that and act as though it were ERROR.
We could implement the clamp either in elog.c or in a GUC assignment
hook. If we do the latter, then SHOW and pg_settings would report the
effective value rather than what you set. That seems a bit cleaner
to me, and not without precedent. As far as the backwards compatibility
angle goes, you can invent scenarios in which either choice could be
argued to break something; but I think the most likely avenue for
trouble is if the visible setting doesn't match the actual behavior.
So I'm leaning to the assign-hook approach; comments?
regards, tom lane
Hi,
On 2018-11-08 10:56:33 -0500, Tom Lane wrote:
OK, so the consensus seems to be that the back branches should continue
to allow you to set client_min_messages = FATAL/PANIC, but then ignore
that and act as though it were ERROR.
Sounds good.
We could implement the clamp either in elog.c or in a GUC assignment
hook. If we do the latter, then SHOW and pg_settings would report the
effective value rather than what you set. That seems a bit cleaner
to me, and not without precedent. As far as the backwards compatibility
angle goes, you can invent scenarios in which either choice could be
argued to break something; but I think the most likely avenue for
trouble is if the visible setting doesn't match the actual behavior.
So I'm leaning to the assign-hook approach; comments?
Seems reasonable.
Greetings,
Andres Freund
On Thu, Nov 8, 2018 at 10:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
OK, so the consensus seems to be that the back branches should continue
to allow you to set client_min_messages = FATAL/PANIC, but then ignore
that and act as though it were ERROR.
Agreed.
We could implement the clamp either in elog.c or in a GUC assignment
hook. If we do the latter, then SHOW and pg_settings would report the
effective value rather than what you set. That seems a bit cleaner
to me, and not without precedent. As far as the backwards compatibility
angle goes, you can invent scenarios in which either choice could be
argued to break something; but I think the most likely avenue for
trouble is if the visible setting doesn't match the actual behavior.
So I'm leaning to the assign-hook approach; comments?
My patch used the check hook, but works either way.
--
Jonah H. Harris
"Jonah H. Harris" <jonah.harris@gmail.com> writes:
On Thu, Nov 8, 2018 at 10:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
We could implement the clamp either in elog.c or in a GUC assignment
hook. If we do the latter, then SHOW and pg_settings would report the
effective value rather than what you set. That seems a bit cleaner
to me, and not without precedent. As far as the backwards compatibility
angle goes, you can invent scenarios in which either choice could be
argued to break something; but I think the most likely avenue for
trouble is if the visible setting doesn't match the actual behavior.
So I'm leaning to the assign-hook approach; comments?
My patch used the check hook, but works either way.
I was deliberately not getting into the detail of which hook to use ;-).
Anyway, pushed with some adjustments and work on the documentation.
Notably, I thought the warning message was inappropriate and
overcomplicated, so I just dropped it. I don't think we really need
anything there.
regards, tom lane
On Thu, Nov 8, 2018 at 5:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
My patch used the check hook, but works either way.
I was deliberately not getting into the detail of which hook to use ;-).
Anyway, pushed with some adjustments and work on the documentation.
Notably, I thought the warning message was inappropriate and
overcomplicated, so I just dropped it. I don't think we really need
anything there.
+1
--
Jonah H. Harris