Thread: Disallow setting client_min_messages > ERROR?
There's a thread on the ODBC list[1] 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] https://www.postgresql.org/message-id/flat/EE586BE92A4AFB45B03310C2A0C0565D6D0EFC17%40G01JPEXMBKW03
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
Attachment
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
Attachment
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