Horton

Thread

Commits

  1. Disallow setting client_min_messages higher than ERROR.

  1. Disallow setting client_min_messages > ERROR?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-11-06T16:19:40Z

    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

  2. Re: Disallow setting client_min_messages > ERROR?

    Andres Freund <andres@anarazel.de> — 2018-11-06T16:31:38Z

    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

  3. Re: Disallow setting client_min_messages > ERROR?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-11-06T16:37:40Z

    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

  4. Re: Disallow setting client_min_messages > ERROR?

    Andres Freund <andres@anarazel.de> — 2018-11-06T16:40:55Z

    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

  5. Re: Disallow setting client_min_messages > ERROR?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-11-06T16:48:03Z

    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

  6. Re: Disallow setting client_min_messages > ERROR?

    Robert Haas <robertmhaas@gmail.com> — 2018-11-06T22:32:01Z

    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

  7. Re: Disallow setting client_min_messages > ERROR?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-11-08T15:56:33Z

    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

  8. Re: Disallow setting client_min_messages > ERROR?

    Andres Freund <andres@anarazel.de> — 2018-11-08T16:06:30Z

    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

  9. Re: Disallow setting client_min_messages > ERROR?

    Jonah H. Harris <jonah.harris@gmail.com> — 2018-11-08T16:09:19Z

    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

  10. Re: Disallow setting client_min_messages > ERROR?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-11-08T22:37:06Z

    "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

  11. Re: Disallow setting client_min_messages > ERROR?

    Jonah H. Harris <jonah.harris@gmail.com> — 2018-11-08T23:24:04Z

    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