Обсуждение: 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

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

"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