Disallow setting client_min_messages > ERROR?

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

Disallow setting client_min_messages > ERROR?

Tom Lane-2
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

Reply | Threaded
Open this post in threaded view
|

Re: Disallow setting client_min_messages > ERROR?

Andres Freund
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

Reply | Threaded
Open this post in threaded view
|

Re: Disallow setting client_min_messages > ERROR?

Tom Lane-2
Andres Freund <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Disallow setting client_min_messages > ERROR?

Andres Freund
On 2018-11-06 11:37:40 -0500, Tom Lane wrote:

> Andres Freund <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Disallow setting client_min_messages > ERROR?

Alvaro Herrera-9
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

Reply | Threaded
Open this post in threaded view
|

Re: Disallow setting client_min_messages > ERROR?

Jonah H. Harris-2
In reply to this post by Tom Lane-2
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


client_min_messages_config_hard.patch (598 bytes) Download Attachment
client_min_messages_config_soft.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Disallow setting client_min_messages > ERROR?

Isaac Morland
On Tue, 6 Nov 2018 at 14:07, Jonah H. Harris <[hidden email]> 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?
Reply | Threaded
Open this post in threaded view
|

Re: Disallow setting client_min_messages > ERROR?

Jonah H. Harris-2
On Tue, Nov 6, 2018 at 2:46 PM Isaac Morland <[hidden email]> wrote:
On Tue, 6 Nov 2018 at 14:07, Jonah H. Harris <[hidden email]> 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


client_min_messages_config_soft-v2.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Disallow setting client_min_messages > ERROR?

Robert Haas
In reply to this post by Alvaro Herrera-9
On Tue, Nov 6, 2018 at 11:48 AM Alvaro Herrera <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Disallow setting client_min_messages > ERROR?

Tom Lane-2
In reply to this post by Andres Freund
Andres Freund <[hidden email]> writes:
> On 2018-11-06 11:37:40 -0500, Tom Lane wrote:
>> Andres Freund <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Disallow setting client_min_messages > ERROR?

Andres Freund
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

Reply | Threaded
Open this post in threaded view
|

Re: Disallow setting client_min_messages > ERROR?

Jonah H. Harris-2
In reply to this post by Tom Lane-2
On Thu, Nov 8, 2018 at 10:56 AM Tom Lane <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Disallow setting client_min_messages > ERROR?

Tom Lane-2
"Jonah H. Harris" <[hidden email]> writes:
> On Thu, Nov 8, 2018 at 10:56 AM Tom Lane <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Disallow setting client_min_messages > ERROR?

Jonah H. Harris-2
On Thu, Nov 8, 2018 at 5:37 PM Tom Lane <[hidden email]> 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