Обсуждение: PL/pgSQL, RAISE and error context

Hi,

By default, PL/pgSQL does not print the error context of a RAISE 
statement, for example:

=# create function foof() returns void as $$ begin raise exception 
'foo'; end $$ language plpgsql;
CREATE FUNCTION

=# create function bar() returns void as $$ begin perform foof(); end $$ 
language plpgsql;
CREATE FUNCTION

=# select bar();
ERROR:  foo
CONTEXT:  SQL statement "SELECT foof()"
PL/pgSQL function "bar" line 1 at PERFORM


I find this extremely surprising, since if you raise the same exception 
(or a DEBUG/NOTICE message) in multiple places, the error context is 
missing valuable information.  With a trivial change the last error 
could be:

=# select bar();
ERROR:  foo
CONTEXT:  PL/pgSQL function "foof" line 1 RAISE
SQL statement "SELECT foof()"
PL/pgSQL function "bar" line 1 at PERFORM

which I find a lot better.

Thoughts?


Regards,
Marko Tiikkaja

On 8/21/13 2:28 PM, I wrote:
> By default, PL/pgSQL does not print the error context of a RAISE
> statement, for example:

An even worse example:

=# create function foof() returns void as $$ begin raise exception 
'foo'; end $$ language plpgsql;
CREATE FUNCTION

=# create function barf() returns void as $$ declare _ record; begin for 
_ in execute 'select foof()' loop end loop; end $$ language plpgsql;
CREATE FUNCTION

=# select barf();
ERROR:  foo
CONTEXT:  PL/pgSQL function "barf" line 1 at FOR over EXECUTE statement

Notice how there's no mention at all about the function the error came 
from, and compare that to:

=# select barf();
ERROR:  foo
CONTEXT:  PL/pgSQL function "foof" line 1 RAISE
PL/pgSQL function "barf" line 1 at FOR over EXECUTE statement



Regards,
Marko Tiikkaja

Marko Tiikkaja <marko@joh.to> writes:
> By default, PL/pgSQL does not print the error context of a RAISE 
> statement, for example:

It used to do so, in the beginning when we first added context-printing.
There were complaints that the result was too verbose; for instance if you
had a RAISE NOTICE inside a loop for progress-monitoring purposes, you'd
get two lines for every one you wanted.  I think if we undid this we'd
get the same complaints again.  I agree that in complicated nests of
functions the location info is more interesting than it is in trivial
cases, but that doesn't mean you're not going to hear such complaints from
people with trivial functions.
        regards, tom lane

On 8/21/13 4:22 PM, Tom Lane wrote:
> Marko Tiikkaja <marko@joh.to> writes:
>> By default, PL/pgSQL does not print the error context of a RAISE
>> statement, for example:
>
> It used to do so, in the beginning when we first added context-printing.
> There were complaints that the result was too verbose; for instance if you
> had a RAISE NOTICE inside a loop for progress-monitoring purposes, you'd
> get two lines for every one you wanted.  I think if we undid this we'd
> get the same complaints again.  I agree that in complicated nests of
> functions the location info is more interesting than it is in trivial
> cases, but that doesn't mean you're not going to hear such complaints from
> people with trivial functions.

They have an option: they can reduce verbosity in their client.  I 
currently don't have any real options.


Regards,
Marko Tiikkaja

On Wed, Aug 21, 2013 at 9:22 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Marko Tiikkaja <marko@joh.to> writes:
>> By default, PL/pgSQL does not print the error context of a RAISE
>> statement, for example:
>
> It used to do so, in the beginning when we first added context-printing.
> There were complaints that the result was too verbose; for instance if you
> had a RAISE NOTICE inside a loop for progress-monitoring purposes, you'd
> get two lines for every one you wanted.  I think if we undid this we'd
> get the same complaints again.  I agree that in complicated nests of
> functions the location info is more interesting than it is in trivial
> cases, but that doesn't mean you're not going to hear such complaints from
> people with trivial functions.

It *is* (apologies for the hijack) too verbose but whatever context
suppressing we added doesn't work in pretty much any interesting case.What is basically needed is for the console to
honor
log_error_verbosity (which I would prefer) or a separate GUC in manage
the console logging verbosity:

set log_error_verbosity = 'terse';
SET

CREATE OR REPLACE FUNCTION Notice(_msg TEXT) RETURNS VOID AS
$$
BEGIN RAISE NOTICE '[%] %', clock_timestamp()::timestamp(0)::text, _msg;
END;
$$ LANGUAGE PLPGSQL;

CREATE OR REPLACE FUNCTION foo() RETURNS VOID AS
$$
BEGIN PERFORM Notice('test');
END;
$$ LANGUAGE PLPGSQL;

-- context will print
postgres=# select foo();
NOTICE:  [2013-08-21 09:52:08] test
CONTEXT:  SQL statement "SELECT Notice('test')"
PL/pgSQL function foo() line 4 at PERFORM

CREATE OR REPLACE FUNCTION bar() RETURNS VOID AS
$$ SELECT Notice('test');
$$ LANGUAGE SQL;

-- context will not print
postgres=# select bar();
NOTICE:  [2013-08-21 09:54:55] test

-- context will  print
CREATE OR REPLACE FUNCTION baz() RETURNS VOID AS
$$ select 0; SELECT Notice('test');
$$ LANGUAGE SQL;

postgres=# select baz();
NOTICE:  [2013-08-21 09:55:26] test
CONTEXT:  SQL function "baz" statement 2

merlin

On 8/21/13 5:05 PM, Merlin Moncure wrote:
> On Wed, Aug 21, 2013 at 9:22 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Marko Tiikkaja <marko@joh.to> writes:
>>> By default, PL/pgSQL does not print the error context of a RAISE
>>> statement, for example:
>>
>> It used to do so, in the beginning when we first added context-printing.
>> There were complaints that the result was too verbose; for instance if you
>> had a RAISE NOTICE inside a loop for progress-monitoring purposes, you'd
>> get two lines for every one you wanted.  I think if we undid this we'd
>> get the same complaints again.  I agree that in complicated nests of
>> functions the location info is more interesting than it is in trivial
>> cases, but that doesn't mean you're not going to hear such complaints from
>> people with trivial functions.
>
> It *is* (apologies for the hijack) too verbose but whatever context
> suppressing we added doesn't work in pretty much any interesting case.
>   What is basically needed is for the console to honor
> log_error_verbosity (which I would prefer) or a separate GUC in manage
> the console logging verbosity:

Why does  \set VERBOSITY 'terse'  not work for you?



Regards,
Marko Tiikkaja

On Wed, Aug 21, 2013 at 10:07 AM, Marko Tiikkaja <marko@joh.to> wrote:
> On 8/21/13 5:05 PM, Merlin Moncure wrote:
>>
>> On Wed, Aug 21, 2013 at 9:22 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>
>>> Marko Tiikkaja <marko@joh.to> writes:
>>>>
>>>> By default, PL/pgSQL does not print the error context of a RAISE
>>>> statement, for example:
>>>
>>>
>>> It used to do so, in the beginning when we first added context-printing.
>>> There were complaints that the result was too verbose; for instance if
>>> you
>>> had a RAISE NOTICE inside a loop for progress-monitoring purposes, you'd
>>> get two lines for every one you wanted.  I think if we undid this we'd
>>> get the same complaints again.  I agree that in complicated nests of
>>> functions the location info is more interesting than it is in trivial
>>> cases, but that doesn't mean you're not going to hear such complaints
>>> from
>>> people with trivial functions.
>>
>>
>> It *is* (apologies for the hijack) too verbose but whatever context
>> suppressing we added doesn't work in pretty much any interesting case.
>>   What is basically needed is for the console to honor
>> log_error_verbosity (which I would prefer) or a separate GUC in manage
>> the console logging verbosity:
>
>
> Why does  \set VERBOSITY 'terse'  not work for you?

Because it can't be controlled mid-function...that would suppress all
context of errors as well as messages and so it's useless.  Also psql
directives for this purpose is a hack anyways -- what if I'm using a
non-psql client?

what I really want is:
SET LOCAL log_console_verbosity = 'x'

merlin

On 2013-08-21 17:18, Merlin Moncure wrote:
> On Wed, Aug 21, 2013 at 10:07 AM, Marko Tiikkaja <marko@joh.to> wrote:
>> Why does  \set VERBOSITY 'terse'  not work for you?
>
> Because it can't be controlled mid-function...that would suppress all
> context of errors as well as messages and so it's useless.

Fair enough.

> what I really want is:
> SET LOCAL log_console_verbosity = 'x'

log_min_messages vs. client_min_messages, so client_error_verbosity 
sounds more appropriate IMO.


Regards,
Marko Tiikkaja

Merlin Moncure <mmoncure@gmail.com> writes:
> On Wed, Aug 21, 2013 at 10:07 AM, Marko Tiikkaja <marko@joh.to> wrote:
>> Why does  \set VERBOSITY 'terse'  not work for you?

> Because it can't be controlled mid-function...that would suppress all
> context of errors as well as messages and so it's useless.  Also psql
> directives for this purpose is a hack anyways -- what if I'm using a
> non-psql client?

> what I really want is:
> SET LOCAL log_console_verbosity = 'x'

There was a protocol design decision a long time ago that verbosity
ought to be controlled on the client side.  If we start suppressing
fields server-side I think we're going to have problems.  In particular,
I'm going to throw the "what if I'm not using psql" argument right back
at you: what's the reason for thinking that a different client/application
would have the identical desires about what fields to see?  It seems
unlikely that a Java application, say, would want the server to be
selective about what information it sends.

I'm entirely prepared to believe that psql's VERBOSITY behavior could
use more options, though.
        regards, tom lane

On Wed, Aug 21, 2013 at 10:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Merlin Moncure <mmoncure@gmail.com> writes:
>> On Wed, Aug 21, 2013 at 10:07 AM, Marko Tiikkaja <marko@joh.to> wrote:
>>> Why does  \set VERBOSITY 'terse'  not work for you?
>
>> Because it can't be controlled mid-function...that would suppress all
>> context of errors as well as messages and so it's useless.  Also psql
>> directives for this purpose is a hack anyways -- what if I'm using a
>> non-psql client?
>
>> what I really want is:
>> SET LOCAL log_console_verbosity = 'x'
>
> There was a protocol design decision a long time ago that verbosity
> ought to be controlled on the client side.  If we start suppressing
> fields server-side I think we're going to have problems.  In particular,
> I'm going to throw the "what if I'm not using psql" argument right back
> at you: what's the reason for thinking that a different client/application
> would have the identical desires about what fields to see?  It seems
> unlikely that a Java application, say, would want the server to be
> selective about what information it sends.

I didn't like that decision then and I don't like it now.  Why should
the protocol mandate that error context always be sent?  Why does this
have anything to do with the protocol at all? Just because we can't
imagine a case where a java application would not want context to be
sent in some or all contexts doesn't mean that operators should not
have control over it being emitted.  What harm could providing an
option possibly cause?

> I'm entirely prepared to believe that psql's VERBOSITY behavior could
> use more options, though.

How would that be structured... \set VERBOSITY 'NOTICE:terse'?

merlin

On Thu, Aug 22, 2013 at 2:08 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Probably we can introduce a new level of verbosity, but I am thinking so
> this behave is reasonable. Everybody who use a VERBOSE level expect lot of
> balast and it show expected info (context of error)
>
> Can be this design good enough for you?

yep :-).

merlin

On 8/22/13 9:08 AM, Pavel Stehule wrote:
> Probably we can introduce a new level of verbosity, but I am thinking so
> this behave is reasonable. Everybody who use a VERBOSE level expect lot of
> balast and it show expected info (context of error)
>
> Can be this design good enough for you?

I like the idea, but I think this should be a new verbosity level.  With 
this patch you would have to go full VERBOSE just to debug PL/pgSQL code 
with NOTICEs and DEBUGs in it, and that output then becomes harder to 
parse with the useless C-code information.


Regards,
Marko Tiikkaja

On 8/23/13 8:38 AM, Pavel Stehule wrote:
> 2013/8/22 Marko Tiikkaja <marko@joh.to>
>> I like the idea, but I think this should be a new verbosity level.  With
>> this patch you would have to go full VERBOSE just to debug PL/pgSQL code
>> with NOTICEs and DEBUGs in it, and that output then becomes harder to parse
>> with the useless C-code information.
>>
>
> do you prepare patch ?

I should have the time to produce one for the September commitfest, but 
if you (or anyone else) want to work on this, I won't object.

My opinion at this very moment is that we should leave the the DEFAULT 
verbosity alone and add a new one (call it COMPACT or such) with the 
suppressed context for non-ERRORs.


Regards,
Marko Tiikkaja

On 23/08/2013 10:36, I wrote:
> On 8/23/13 8:38 AM, Pavel Stehule wrote:
>> do you prepare patch ?
>
> I should have the time to produce one for the September commitfest, but
> if you (or anyone else) want to work on this, I won't object.
>
> My opinion at this very moment is that we should leave the the DEFAULT
> verbosity alone and add a new one (call it COMPACT or such) with the
> suppressed context for non-ERRORs.

Well, turns out there isn't really any way to preserve complete
backwards compatibility if we want to do this change.

The attached patch (based on Pavel's patch) changes the default to be
slightly more verbose (the CONTEXT lines which were previously omitted
will be visible), but adds a new PGVerbosity called COMPACT which
suppresses CONTEXT in non-error messages.  Now DEFAULT will be more
useful when debugging PL/PgSQL, and people who are annoyed by the new
behaviour can use the COMPACT mode.

Any thoughts?



Regards,
Marko Tiikkaja

On Sat, 2013-09-14 at 04:58 +0200, Marko Tiikkaja wrote:
> The attached patch (based on Pavel's patch) changes the default to be 
> slightly more verbose (the CONTEXT lines which were previously
> omitted 
> will be visible), but adds a new PGVerbosity called COMPACT which 
> suppresses CONTEXT in non-error messages.  Now DEFAULT will be more 
> useful when debugging PL/PgSQL, and people who are annoyed by the new 
> behaviour can use the COMPACT mode. 

Your patch fails the regression tests.

On 15/09/2013 04:05, Peter Eisentraut wrote:
> On Sat, 2013-09-14 at 04:58 +0200, Marko Tiikkaja wrote:
>> The attached patch (based on Pavel's patch) changes the default to be
>> slightly more verbose (the CONTEXT lines which were previously
>> omitted
>> will be visible), but adds a new PGVerbosity called COMPACT which
>> suppresses CONTEXT in non-error messages.  Now DEFAULT will be more
>> useful when debugging PL/PgSQL, and people who are annoyed by the new
>> behaviour can use the COMPACT mode.
>
> Your patch fails the regression tests.

Attached is an updated patch with the regression test fixes.  No other
changes included.


Regards,
Marko Tiikkaja

On 15/09/2013 13:50, I wrote:
> On 15/09/2013 04:05, Peter Eisentraut wrote:
>> On Sat, 2013-09-14 at 04:58 +0200, Marko Tiikkaja wrote:
>>> The attached patch (based on Pavel's patch) changes the default to be
>>> slightly more verbose (the CONTEXT lines which were previously
>>> omitted
>>> will be visible), but adds a new PGVerbosity called COMPACT which
>>> suppresses CONTEXT in non-error messages.  Now DEFAULT will be more
>>> useful when debugging PL/PgSQL, and people who are annoyed by the new
>>> behaviour can use the COMPACT mode.
>>
>> Your patch fails the regression tests.
>
> Attached is an updated patch with the regression test fixes.  No other
> changes included.

Hmm.  I just noticed there's something weird going on in the select_view 
test.  I'm investigating this currently.


Regards,
Marko Tiikkaja

On 15/09/2013 13:58, I wrote:
> Hmm.  I just noticed there's something weird going on in the select_view
> test.  I'm investigating this currently.

Seems that there's some magic going on and I overwrote the expected
results of the wrong file.  However, I can't figure out how one is
supposed to be getting the output of expected/select_views.out, nor do I
find this documented anywhere (I know xml has a similar thing so I tried
grepping around for XML, to no avail).

Here's an updated patch, but I think expected/select_views.out is still
broken.


Regards,
Marko Tiikkaja

On Sun, 2013-09-15 at 14:28 +0200, Marko Tiikkaja wrote:
> On 15/09/2013 13:58, I wrote:
> > Hmm.  I just noticed there's something weird going on in the select_view
> > test.  I'm investigating this currently.
> 
> Seems that there's some magic going on and I overwrote the expected 
> results of the wrong file.  However, I can't figure out how one is 
> supposed to be getting the output of expected/select_views.out, nor do I 
> find this documented anywhere (I know xml has a similar thing so I tried 
> grepping around for XML, to no avail).
> 
> Here's an updated patch, but I think expected/select_views.out is still 
> broken.

You patch still fails the plperl regression tests.

I don't see a failure with select_views.  Your issue might be that you
updated expected/select_views_1.out but not expected/select_views.out.
This documentation might help:
http://www.postgresql.org/docs/devel/static/regress-variant.html

Hello,

I just heard that there's going to be a fifth CF for 9.5 so I'm trying 
to gather all the patches I'd like to see in 9.5..

On 8/23/13 10:36 AM, I wrote:
> My opinion at this very moment is that we should leave the the DEFAULT
> verbosity alone and add a new one (call it COMPACT or such) with the
> suppressed context for non-ERRORs.

I wonder if a better option would be to add a GUC to control this from 
the server side.  plpgsql.suppress_simple_error_context or such, 
defaulting to false to maintain full backwards compatibility.  That 
could be set to true for development installations and for client 
programs which only care about having all information available, rather 
than readability or aesthetics.

Or is that a stupid idea?  I just think hacking libpq for something like 
this is a huge overkill.


.marko

On 1/22/15 6:03 PM, Pavel Stehule wrote:
> 2015-01-22 12:37 GMT+01:00 Marko Tiikkaja <marko@joh.to>:
>> Or is that a stupid idea?  I just think hacking libpq for something like
>> this is a huge overkill.
>>
>
> I don't think so only plpgsql  solution is satisfactory idea. There are
> some mix plpgsql / plperl ... application - and it isn't possible to remove
> error context from only one language.

Yeah, not in libpq it isn't.  Thing is, PL/PgSQL already is the 
exception here, since it's the only language which does this error 
message suppression.  So if people did think this suppression was a good 
idea, only the people using PL/PgSQL were vocal enough to get the 
behavior changed.  I'm not looking to change that.

I can see where it's a lot nicer not to have the context visible for 
people who only care about the contents of the message, but the way it's 
done in PL/PgSQL right now is just not good enough.  On the other hand, 
the backwards compatibility breakage of doing this in libpq is quite 
extensive.  The most simple option seems to be to just allow a GUC to 
change PL/PgSQL's behavior to match what all other PLs are doing.


.marko

On 1/26/15 1:14 PM, Pavel Stehule wrote:
> 2015-01-26 13:02 GMT+01:00 Marko Tiikkaja <marko@joh.to>:
>> I can see where it's a lot nicer not to have the context visible for
>> people who only care about the contents of the message, but the way it's
>> done in PL/PgSQL right now is just not good enough.  On the other hand, the
>> backwards compatibility breakage of doing this in libpq is quite
>> extensive.  The most simple option seems to be to just allow a GUC to
>> change PL/PgSQL's behavior to match what all other PLs are doing.
>>
>
> libpq was changed more time - there is still a open task about a protocol
> change.
>
> I afraid about some unexpected side effects of your proposal if somebody
> mix languages - these side effects should not be critical

I have no idea what you're talking about.  What kind of side effects?

> - but on second
> hand current behave is not critical too - we can wait.

I think the current behavior is almost unacceptable.  It makes debugging 
in some cases really, really difficult.


.marko

On 1/26/15 1:44 PM, Pavel Stehule wrote:
> 2015-01-26 13:39 GMT+01:00 Marko Tiikkaja <marko@joh.to>:
>> On 1/26/15 1:14 PM, Pavel Stehule wrote:
>>> I afraid about some unexpected side effects of your proposal if somebody
>>> mix languages - these side effects should not be critical
>>>
>>
>> I have no idea what you're talking about.  What kind of side effects?
>>
>
> what will be a error context if plpgsql calls a plperl function that raises
> a exception
> what will be a error context if plperl calls a plpgsql functions that
> raises a exception

I fail to see the point.  How would that be different from what happens 
today?  Remember, PL/PgSQL only suppresses the *topmost* stack frame, 
and only when using RAISE from within a PL/PgSQL function.


.m

Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2015-01-26 14:02 GMT+01:00 Marko Tiikkaja <marko@joh.to>:
> I am thinking, so solution

>      /* if we are doing RAISE, don't report its location */
>     if (estate->err_text == raise_skip_msg)
>         return;

> is too simple, and this part should be fixed. This change can be done by on
> plpgsql or libpq side. This is bug, and it should be fixed.

Doing this in libpq is utterly insane.  It has not got sufficient context
to do anything intelligent.  The fact that it's not intelligent is exposed
by the regression test changes that the proposed patch causes, most of
which do not look like improvements.

Another problem is that past requests to change this behavior have
generally been to the effect that people wanted *more* context suppressed
not less, ie they didn't want any CONTEXT lines at all on certain
messages.  So the proposed patch seems to me to be going in exactly the
wrong direction.

The design I thought had been agreed on was to add some new option to
plpgsql's RAISE command which would cause suppression of all CONTEXT lines
not just the most closely nested one.  You could argue about whether the
behavior needs to be 100% backwards compatible or not --- if so, perhaps
it could be a three-way option all, none, or one line, defaulting to the
last for backwards compatibility.
        regards, tom lane

On 1/26/15 9:46 AM, Pavel Stehule wrote:
>
>     The design I thought had been agreed on was to add some new option to
>     plpgsql's RAISE command which would cause suppression of all CONTEXT lines
>     not just the most closely nested one.  You could argue about whether the
>     behavior needs to be 100% backwards compatible or not --- if so, perhaps
>     it could be a three-way option all, none, or one line, defaulting to the
>     last for backwards compatibility.
>
>
>   I see a problem what should be default behave. When I raise NOTICE, then I don't need (don't would) to see CONTEXT
lines,When I raise EXCEPTION, then I usually would to see CONTEXT lines.
 

FWIW, that's the case I almost always run into: I turn on some debugging which means I know where the RAISE is coming
from,but now I'm flooded with CONTEXT lines. You could do that with an option to RAISE, but that seems like a lot of
extracoding work for little gain. Perhaps it'd be worth creating client_min_context and log_min_context GUCs...
 

Another option that I think would work well is that you only provide context for the first call within a "block" of
code.For plpgsql that would be a function, but maybe it'd be better to just do this per-subtransaction.
 

I do agree that this needs to work across the board, not just for plpgsql.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 4/2/15 9:37 AM, Pavel Stehule wrote:
>> estate->err_text = stmt->elog_level == ERROR ? estate->err_text :
>> raise_skip_msg  ;
>
> Can we do this simple change? It will produce a stackinfo for exceptions
> and it will not to make mad developers by lot of useless content.

I'm not sure everyone agrees with this to be honest, myself included.

I think the best way to do this would be to have an option for RAISE to 
suppress the context *regardless of nesting depth*, but show the full 
context by default for ERRORs.  For NOTICEs and WARNINGs I don't care 
much what the default will be; perhaps just full backwards compatibility 
could work there.


.m

On Thu, Apr 23, 2015 at 4:56 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> I don't see a contradiction. There is clean agreement, so ERROR level should
> to show the context. NOTICE and WARNINGs doesn't need it - and there is a
> backward compatibility and usability reasons don't do it.

Whether notices and warnings need it is a matter of opinion.  I don't
think your idea is bad, and it might be a good rule of thumb in many
cases, but I slightly prefer Marko's approach of adding a new option.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Apr 23, 2015 at 9:55 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> On Thu, Apr 23, 2015 at 4:56 AM, Pavel Stehule <pavel.stehule@gmail.com>
>> wrote:
>> > I don't see a contradiction. There is clean agreement, so ERROR level
>> > should
>> > to show the context. NOTICE and WARNINGs doesn't need it - and there is
>> > a
>> > backward compatibility and usability reasons don't do it.
>>
>> Whether notices and warnings need it is a matter of opinion.  I don't
>> think your idea is bad, and it might be a good rule of thumb in many
>> cases, but I slightly prefer Marko's approach of adding a new option.
>
> I am not sure if I understand to you.
>
> please, can you write more about your idea?

Your idea, as I understand it, is that for logs at severity levels
lower than ERROR, we can always emit the context, because it's not
necessary.  But I'm not sure that's right: some people might find that
context helpful.  If, as Marko proposes, we add an explicit option,
then everyone can choose the behavior that is right for them.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Entering the discussion because this is a huge pain for me in my daily
work as well.

This is not a reply to any specific post in this thread, but my first
message in the thread.

I see a great value in providing both a GUC and a new RAISE syntax.
The different benefits of the two are maybe obvious, but perhaps worth
pointing out:
GUC: Good because you don't have to change any existing code.
RAISE syntax: Good because you can control exactly what message should
be emitted or not be emitted at that line of code.

I think preserving backwards compatibility is very important.
Not changing the default is not a problem for me, as long as it can be
overridden.

Whatever the default behaviour is, I think the need expressed by all
users in this thread boils down to any of these two sentences:

"I want CONTEXT to be (DISPLAYED|SUPPRESSED) for (ALL|ONLY THIS LINE)
RAISE (NOTICE|WARNING|ERROR)"
OR
"I don't want to change the default current behaviour of CONTEXT"

So we basically need a boolean setting value, where:
NULL means the default behaviour
TRUE means DISPLAY CONTEXT
FALSE means SUPPRESS CONTEXT

And the (ALL|ONLY THIS) part translates into using,
* a GUC to change behaviour for ALL lines of code,
* or using the RAISE syntax to change the behaviour of ONLY THIS line of code.

And then we have the different message levels, for which CONTEXT is
sometimes desirable in some situations:
* The RAISE syntax allows controlling any message level in a natural
way, as the message level is part of the syntax.
* Allowing the same control using GUC would mean the message level
would need to be part of the GUC key name, which means either add
multiple GUCs, one for each message level, or only allow controlling
the most important one and ignore the possibly need to control the
other message levels.

If it would be possible to somehow combine multiple message levels in
the same GUC, that would solve the latter problem.

We already have comma separated values for many GUCs, so maybe we
could use that approach here as well.

It looks like adding these two GUCs would meet the demands of all users:

suppress_context_messages (enum)
display_context_messages (enum)

This would allow doing something crazy as:

suppress_context_messages = warning,error
display_context_messages = notice

On Fri, Apr 24, 2015 at 6:11 AM, Joel Jacobson <joel@trustly.com> wrote:
> Entering the discussion because this is a huge pain for me in my daily
> work as well.
>
> This is not a reply to any specific post in this thread, but my first
> message in the thread.
>
> I see a great value in providing both a GUC and a new RAISE syntax.
> The different benefits of the two are maybe obvious, but perhaps worth
> pointing out:
> GUC: Good because you don't have to change any existing code.
> RAISE syntax: Good because you can control exactly what message should
> be emitted or not be emitted at that line of code.
>
> I think preserving backwards compatibility is very important.
> Not changing the default is not a problem for me, as long as it can be
> overridden.
>
> Whatever the default behaviour is, I think the need expressed by all
> users in this thread boils down to any of these two sentences:
>
> "I want CONTEXT to be (DISPLAYED|SUPPRESSED) for (ALL|ONLY THIS LINE)
> RAISE (NOTICE|WARNING|ERROR)"
> OR
> "I don't want to change the default current behaviour of CONTEXT"
>
> So we basically need a boolean setting value, where:
> NULL means the default behaviour
> TRUE means DISPLAY CONTEXT
> FALSE means SUPPRESS CONTEXT
>
> And the (ALL|ONLY THIS) part translates into using,
> * a GUC to change behaviour for ALL lines of code,
> * or using the RAISE syntax to change the behaviour of ONLY THIS line of code.
>
> And then we have the different message levels, for which CONTEXT is
> sometimes desirable in some situations:
> * The RAISE syntax allows controlling any message level in a natural
> way, as the message level is part of the syntax.
> * Allowing the same control using GUC would mean the message level
> would need to be part of the GUC key name, which means either add
> multiple GUCs, one for each message level, or only allow controlling
> the most important one and ignore the possibly need to control the
> other message levels.
>
> If it would be possible to somehow combine multiple message levels in
> the same GUC, that would solve the latter problem.
>
> We already have comma separated values for many GUCs, so maybe we
> could use that approach here as well.
>
> It looks like adding these two GUCs would meet the demands of all users:
>
> suppress_context_messages (enum)
> display_context_messages (enum)
>
> This would allow doing something crazy as:
>
> suppress_context_messages = warning,error
> display_context_messages = notice

This is a very flexible proposal, but it's a tremendous amount of
machinery for what's really a very minor issue.  If we added two GUCs
for every comparably important issue, we'd have about 40,000 of them.

I suggest we add the RAISE syntax first, because everybody agrees on
that.  Then, we can argue about the other stuff.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Apr 24, 2015 at 1:16 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> This would allow doing something crazy as:
>>
>> suppress_context_messages = warning,error
>> display_context_messages = notice
>
> This is a very flexible proposal, but it's a tremendous amount of
> machinery for what's really a very minor issue.  If we added two GUCs
> for every comparably important issue, we'd have about 40,000 of them.

I agree. The one-dimensional GUC syntax is not well suited for
multi-dimensional config settings. And that's a good thing mostly I
think. It would be a nightmare if the config file values could in JSON
format, it's good they are simple.

But I'm thinking maybe we could improve the config file syntax for the
general case when you have multiple things you want to control, in
this case the message levels, and for each such thing, you want to
turn something on/off, in this case the CONTEXT. Maybe we could simply
use plus "+" and minus "-" to mean "on" and "off"?

Example:

context_messages = -warning, -error, +notice

On Fri, Apr 24, 2015 at 6:07 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> Example:
>>
>> context_messages = -warning, -error, +notice
>
>
> I prefer your first proposal - and there is a precedent for plpgsql -
> plpgsql_extra_checks
>
> It is clean for anybody. +-identifiers looks like horrible httpd config. :)

I have to agree on that :) Just thought this is the best we can do if
we want to reduce the number of GUCs to a minimum.

<div dir="ltr"><div class="gmail_extra"><br />On Sun, Apr 26, 2015 at 4:19 AM, Pavel Stehule <<a
href="mailto:pavel.stehule@gmail.com">pavel.stehule@gmail.com</a>>wrote:<br />><br />> Hi<br />><br />>
Ireduced this patch, little bit cleaned - now it is based on plpgsql GUC display_context_min_messages - like
client_min_messages,log_min_messages.<br />><br /><br /></div><div class="gmail_extra">What you think just
"context_min_messages"?<br /></div><div class="gmail_extra"><br /></div><div class="gmail_extra">Regards,<br /><br
/></div><divclass="gmail_extra">--<br />Fabrízio de Royes Mello<br />Consultoria/Coaching PostgreSQL<br />>>
Timbira:<a href="http://www.timbira.com.br">http://www.timbira.com.br</a><br />>> Blog: <a
href="http://fabriziomello.github.io">http://fabriziomello.github.io</a><br/>>> Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/>>> Github: <a
href="http://github.com/fabriziomello">http://github.com/fabriziomello</a></div></div>

On 4/27/15 6:08 PM, Fabrízio de Royes Mello wrote:
> On Sun, Apr 26, 2015 at 4:19 AM, Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
>> I reduced this patch, little bit cleaned - now it is based on plpgsql GUC
> display_context_min_messages - like client_min_messages, log_min_messages.
>
> What you think just "context_min_messages" ?

That sounds weird.  log_min_messages are the messages sent to the log; 
client_min_messages are sent to the client.  context_min_messages are 
not sent to a "context", whatever that would mean.


.m

On 4/27/15 11:47 AM, Joel Jacobson wrote:
> On Mon, Apr 27, 2015 at 6:14 PM, Marko Tiikkaja <marko@joh.to
> <mailto:marko@joh.to>> wrote:
>
>     That sounds weird.  log_min_messages are the messages sent to the
>     log; client_min_messages are sent to the client.
>     context_min_messages are not sent to a "context", whatever that
>     would mean.
>
>
> Good point.
>
> I think it can't be any clearer than the proposed
> "plpgsql.display_context_min_messages"

client_min_context. It's doing the same thing as min_messages does, just 
for context instead of the message.

Or does this affect client and log the same way?
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 4/28/15 1:16 AM, Pavel Stehule wrote:
>
>         I think it can't be any clearer than the proposed
>         "plpgsql.display_context_min_messages"
>
>
>     client_min_context. It's doing the same thing as min_messages does,
>     just for context instead of the message.
>
>     Or does this affect client and log the same way?
>
>
> it affect client and log together
>
> maybe "min_context"

+1
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

Hi Pavel,

This doesn't seem to be what I thought we had agreed on.  For example:

=# create function barf() returns void as $$ begin raise notice without 
context 'hello world'; end $$ language plpgsql;
CREATE FUNCTION
=# create function foof() returns void as $$ begin perform barf(); end 
$$ language plpgsql;
CREATE FUNCTION
=# select foof();
NOTICE:  hello world
CONTEXT:  SQL statement "SELECT barf()"
PL/pgSQL function foof() line 1 at PERFORM

It's not only clear that WITHOUT CONTEXT didn't really work here, but it 
also had absolutely no effect since the context within barf() is also 
displayed.


.m

On 01/26/2015 05:14 PM, Tom Lane wrote:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> 2015-01-26 14:02 GMT+01:00 Marko Tiikkaja <marko@joh.to>:
>> I am thinking, so solution
>
>>       /* if we are doing RAISE, don't report its location */
>>      if (estate->err_text == raise_skip_msg)
>>          return;
>
>> is too simple, and this part should be fixed. This change can be done by on
>> plpgsql or libpq side. This is bug, and it should be fixed.
>
> Doing this in libpq is utterly insane.  It has not got sufficient context
> to do anything intelligent.  The fact that it's not intelligent is exposed
> by the regression test changes that the proposed patch causes, most of
> which do not look like improvements.

I think doing this in libpq (or psql) is the way to go. How can the
server know if the client wants to display context information? We just
have to make sure the client has enough information to make a smart
decision. If the client doesn't have enough information today, then
let's work on that.

Note that Marko's patch didn't change libpq's default printing mode,
which is why you got all the extra CONTEXT lines in the regression tests
that were not there before. Just as if we just removed the suppression
from PL/pgSQL and did nothing else. I think we need to also change the
default behaviour to not print CONTEXT lines for NOTICE-level messages,
getting us closer to the current behaviour again.

If you run the regression tests in the "compact" verbosity, the
regression test output changes look quite sensible to me. See attached.

> Another problem is that past requests to change this behavior have
> generally been to the effect that people wanted *more* context suppressed
> not less, ie they didn't want any CONTEXT lines at all on certain
> messages.  So the proposed patch seems to me to be going in exactly the
> wrong direction.

After changing the default to "compact", it prints less CONTEXT lines.

> The design I thought had been agreed on was to add some new option to
> plpgsql's RAISE command which would cause suppression of all CONTEXT lines
> not just the most closely nested one.

I don't understand how you came to that conclusion. In particular, see
http://www.postgresql.org/message-id/6656.1377100039@sss.pgh.pa.us. If
you changed your mind, you forgot to tell why.

- Heikki

On Tue, Jul 7, 2015 at 8:13 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 01/26/2015 05:14 PM, Tom Lane wrote:
>>
>> Pavel Stehule <pavel.stehule@gmail.com> writes:
>>>
>>> 2015-01-26 14:02 GMT+01:00 Marko Tiikkaja <marko@joh.to>:
>>> I am thinking, so solution
>>
>>
>>>       /* if we are doing RAISE, don't report its location */
>>>      if (estate->err_text == raise_skip_msg)
>>>          return;
>>
>>
>>> is too simple, and this part should be fixed. This change can be done by
>>> on
>>> plpgsql or libpq side. This is bug, and it should be fixed.
>>
>>
>> Doing this in libpq is utterly insane.  It has not got sufficient context
>> to do anything intelligent.  The fact that it's not intelligent is exposed
>> by the regression test changes that the proposed patch causes, most of
>> which do not look like improvements.
>
> How can the server know if the client wants to display context information?

It doesn't have to if the behavior is guarded with a GUC.  I just
don't understand what all the fuss is about.  The default behavior of
logging that is well established by other languages (for example java)
that manage error stack for you should be to:

*) Give stack trace when an uncaught exception is thrown
*) Do not give stack trace in all other logging cases unless asked for

I would be happy to show you the psql redirected output logs from my
nightly server processes that spew into the megabytes because of
logging various high level steps (did this, did that).   I can't throw
the verbose switch to terse because if the error happens to be
'Division by Zero', or some other difficult to trace problem then I'm
sunk.  I believe the protocol decision to 'always send context' needs
to be revisited; if your server-side codebase is large and heavily
nested it makes logging an expensive operation even if the client
strips off the log.

plpgsql.min_context seems like the ideal solution to this problem; it
can be managed on the server or the client and does not require new
syntax.  If we require syntax to slip and and out of debugging type
operations the solution has missed the mark IMNSHO.

merlin

On 07/07/2015 04:56 PM, Merlin Moncure wrote:
> On Tue, Jul 7, 2015 at 8:13 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> On 01/26/2015 05:14 PM, Tom Lane wrote:
>>>
>>> Pavel Stehule <pavel.stehule@gmail.com> writes:
>>>>
>>>> 2015-01-26 14:02 GMT+01:00 Marko Tiikkaja <marko@joh.to>:
>>>> I am thinking, so solution
>>>
>>>
>>>>        /* if we are doing RAISE, don't report its location */
>>>>       if (estate->err_text == raise_skip_msg)
>>>>           return;
>>>
>>>
>>>> is too simple, and this part should be fixed. This change can be done by
>>>> on
>>>> plpgsql or libpq side. This is bug, and it should be fixed.
>>>
>>>
>>> Doing this in libpq is utterly insane.  It has not got sufficient context
>>> to do anything intelligent.  The fact that it's not intelligent is exposed
>>> by the regression test changes that the proposed patch causes, most of
>>> which do not look like improvements.
>>
>> How can the server know if the client wants to display context information?
>
> It doesn't have to if the behavior is guarded with a GUC.  I just
> don't understand what all the fuss is about.  The default behavior of
> logging that is well established by other languages (for example java)
> that manage error stack for you should be to:
>
> *) Give stack trace when an uncaught exception is thrown
> *) Do not give stack trace in all other logging cases unless asked for

Java's exception handling is so different from PostgreSQL's errors that 
I don't think there's much to be learned from that. But I'll bite:

First of all, Java's exceptions always contain a stack trace. It's up to 
you when you catch an exception to decide whether to print it or not. 
"try { ... } catch (Exception e) { e.printStackTrace() }" is fairly 
common, actually. There is nothing like a NOTICE in Java, i.e. an 
exception that's thrown but doesn't affect the control flow. The best I 
can think of is System.out.println(), which of course has no stack trace 
attached to it.

Perhaps you're arguing that NOTICE is more like printing to stderr, and 
should never contain any context information. I don't think that would 
be an improvement. It's very handy to have the context information 
available if don't know where a NOTICE is coming from, even if in most 
cases you're not interested in it.

This is really quite different from a programming language's exception 
handling. First, there's a server, which produces the errors, and a 
separate client, which displays them. You cannot catch an exception in 
the client.

BTW, let me throw in one use case to consider. We've been talking about 
psql, and what to print, but imagine a more sophisticated client like 
pgAdmin. It's not limited to either printing the context or not. It 
could e.g. hide the context information of all messages when they occur, 
but if you double-click on it, it's expanded to show all the context, 
location and all. You can't do that if the server doesn't send the 
context information in the first place.

> I would be happy to show you the psql redirected output logs from my
> nightly server processes that spew into the megabytes because of
> logging various high level steps (did this, did that).

Oh, I believe you. I understand what the problem is, we're only talking 
about how best to address it.
- Heikki

On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> It doesn't have to if the behavior is guarded with a GUC.  I just
>> don't understand what all the fuss is about.  The default behavior of
>> logging that is well established by other languages (for example java)
>> that manage error stack for you should be to:
>>
>> *) Give stack trace when an uncaught exception is thrown
>> *) Do not give stack trace in all other logging cases unless asked for
>
> what is RAISE EXCEPTION - first or second case?

First: RAISE (unless caught) is no different than any other kind of error.

On Tue, Jul 7, 2015 at 9:42 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 07/07/2015 04:56 PM, Merlin Moncure wrote:
>> It doesn't have to if the behavior is guarded with a GUC.  I just
>> don't understand what all the fuss is about.  The default behavior of
>> logging that is well established by other languages (for example java)
>> that manage error stack for you should be to:
>>
>> *) Give stack trace when an uncaught exception is thrown
>> *) Do not give stack trace in all other logging cases unless asked for
>
> Java's exception handling is so different from PostgreSQL's errors that I
> don't think there's much to be learned from that. But I'll bite:
>
> First of all, Java's exceptions always contain a stack trace. It's up to you
> when you catch an exception to decide whether to print it or not. "try { ...
> } catch (Exception e) { e.printStackTrace() }" is fairly common, actually.
> There is nothing like a NOTICE in Java, i.e. an exception that's thrown but
> doesn't affect the control flow. The best I can think of is
> System.out.println(), which of course has no stack trace attached to it.

exactly.

> Perhaps you're arguing that NOTICE is more like printing to stderr, and
> should never contain any context information. I don't think that would be an
> improvement. It's very handy to have the context information available if
> don't know where a NOTICE is coming from, even if in most cases you're not
> interested in it.

That's exactly what I'm arguing.  NOTICE (and WARNING) are for
printing out information to client side logging; it's really the only
tool we have for that purpose and it fits that role perfectly.  Of
course, you may want to have NOTICE print context, especially when
debugging, but some control over that would be nice and in most cases
it's really not necessary.  I really don't understand the objection to
offering control over that behavior although I certainly understand
wanting to keep the default behavior as it currently is.

> This is really quite different from a programming language's exception
> handling. First, there's a server, which produces the errors, and a separate
> client, which displays them. You cannot catch an exception in the client.
>
> BTW, let me throw in one use case to consider. We've been talking about
> psql, and what to print, but imagine a more sophisticated client like
> pgAdmin. It's not limited to either printing the context or not. It could
> e.g. hide the context information of all messages when they occur, but if
> you double-click on it, it's expanded to show all the context, location and
> all. You can't do that if the server doesn't send the context information in
> the first place.
>
>> I would be happy to show you the psql redirected output logs from my
>> nightly server processes that spew into the megabytes because of
>> logging various high level steps (did this, did that).
>
> Oh, I believe you. I understand what the problem is, we're only talking
> about how best to address it.

Yeah.  For posterity, a psql based solution would work fine for me,
but a server side solution has a lot of advantages (less protocol
chatter, more configurability, keeping libpq/psql light).

merlin

On Wed, Jul 8, 2015 at 1:35 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> 2015-07-07 18:15 GMT+02:00 Merlin Moncure <mmoncure@gmail.com>:
>>
>> On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule <pavel.stehule@gmail.com>
>> wrote:
>> >> It doesn't have to if the behavior is guarded with a GUC.  I just
>> >> don't understand what all the fuss is about.  The default behavior of
>> >> logging that is well established by other languages (for example java)
>> >> that manage error stack for you should be to:
>> >>
>> >> *) Give stack trace when an uncaught exception is thrown
>> >> *) Do not give stack trace in all other logging cases unless asked for
>> >
>> > what is RAISE EXCEPTION - first or second case?
>>
>> First: RAISE (unless caught) is no different than any other kind of error.
>>
>> On Tue, Jul 7, 2015 at 9:42 AM, Heikki Linnakangas <hlinnaka@iki.fi>
>> wrote:
>> > On 07/07/2015 04:56 PM, Merlin Moncure wrote:
>> >> It doesn't have to if the behavior is guarded with a GUC.  I just
>> >> don't understand what all the fuss is about.  The default behavior of
>> >> logging that is well established by other languages (for example java)
>> >> that manage error stack for you should be to:
>> >>
>> >> *) Give stack trace when an uncaught exception is thrown
>> >> *) Do not give stack trace in all other logging cases unless asked for
>> >
>> > Java's exception handling is so different from PostgreSQL's errors that
>> > I
>> > don't think there's much to be learned from that. But I'll bite:
>> >
>> > First of all, Java's exceptions always contain a stack trace. It's up to
>> > you
>> > when you catch an exception to decide whether to print it or not. "try {
>> > ...
>> > } catch (Exception e) { e.printStackTrace() }" is fairly common,
>> > actually.
>> > There is nothing like a NOTICE in Java, i.e. an exception that's thrown
>> > but
>> > doesn't affect the control flow. The best I can think of is
>> > System.out.println(), which of course has no stack trace attached to it.
>>
>> exactly.
>>
>> > Perhaps you're arguing that NOTICE is more like printing to stderr, and
>> > should never contain any context information. I don't think that would
>> > be an
>> > improvement. It's very handy to have the context information available
>> > if
>> > don't know where a NOTICE is coming from, even if in most cases you're
>> > not
>> > interested in it.
>>
>> That's exactly what I'm arguing.  NOTICE (and WARNING) are for
>> printing out information to client side logging; it's really the only
>> tool we have for that purpose and it fits that role perfectly.  Of
>> course, you may want to have NOTICE print context, especially when
>> debugging, but some control over that would be nice and in most cases
>> it's really not necessary.  I really don't understand the objection to
>> offering control over that behavior although I certainly understand
>> wanting to keep the default behavior as it currently is.
>>
>> > This is really quite different from a programming language's exception
>> > handling. First, there's a server, which produces the errors, and a
>> > separate
>> > client, which displays them. You cannot catch an exception in the
>> > client.
>> >
>> > BTW, let me throw in one use case to consider. We've been talking about
>> > psql, and what to print, but imagine a more sophisticated client like
>> > pgAdmin. It's not limited to either printing the context or not. It
>> > could
>> > e.g. hide the context information of all messages when they occur, but
>> > if
>> > you double-click on it, it's expanded to show all the context, location
>> > and
>> > all. You can't do that if the server doesn't send the context
>> > information in
>> > the first place.
>> >
>> >> I would be happy to show you the psql redirected output logs from my
>> >> nightly server processes that spew into the megabytes because of
>> >> logging various high level steps (did this, did that).
>> >
>> > Oh, I believe you. I understand what the problem is, we're only talking
>> > about how best to address it.
>>
>> Yeah.  For posterity, a psql based solution would work fine for me,
>> but a server side solution has a lot of advantages (less protocol
>> chatter, more configurability, keeping libpq/psql light).
>
>
> After some work on reduced version of "plpgsql.min_context" patch I am
> inclining to think so ideal solution needs more steps - because this issue
> has more than one dimension.
>
> There are two independent issues:
>
> 1. old plpgsql workaround that reduced the unwanted call stack info for
> RAISE NOTICE. Negative side effect of this workaround is missing context
> info about the RAISE command that raises the exception. We know a function,
> but we don't know a line of related RAISE statement. The important is fact,
> so NOTICE doesn't bubble to up. So this workaround was relative successful
> without to implement some filtering on client or log side.
>
> 2. second issue is general suppressing context info for interactive client
> or for log.
>
> These issues should be solved separately, because solution for @2 doesn't
> fix @1, and @1 is too local for @2.
>
> So what we can do?
>
> 1. remove current plpgsql workaround - and implement client_min_context and
> log_min_context
> 2. implement plpgsql.min_context, and client_min_context and log_min_context
>
> @1 is consistent, but isn't possible to configure same behave as was before
>
> @2 is difficult in definition what plpgsql.min_context should to  really do
> - and what is relation to client_min_context and log_min_context, but I can
> prepare configuration, that is fully compatible.
>
> Comments, any other ideas?
>
> Personally, I prefer @1 as general solution, that will work for all PL

+1

merlin

On Wed, Jul 8, 2015 at 4:39 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> 2015-07-08 8:35 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:
>>
>>
>>
>> 2015-07-07 18:15 GMT+02:00 Merlin Moncure <mmoncure@gmail.com>:
>>>
>>> On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule <pavel.stehule@gmail.com>
>>> wrote:
>>> >> It doesn't have to if the behavior is guarded with a GUC.  I just
>>> >> don't understand what all the fuss is about.  The default behavior of
>>> >> logging that is well established by other languages (for example java)
>>> >> that manage error stack for you should be to:
>>> >>
>>> >> *) Give stack trace when an uncaught exception is thrown
>>> >> *) Do not give stack trace in all other logging cases unless asked for
>>> >
>>> > what is RAISE EXCEPTION - first or second case?
>>>
>>> First: RAISE (unless caught) is no different than any other kind of
>>> error.
>>>
>>> On Tue, Jul 7, 2015 at 9:42 AM, Heikki Linnakangas <hlinnaka@iki.fi>
>>> wrote:
>>> > On 07/07/2015 04:56 PM, Merlin Moncure wrote:
>>> >> It doesn't have to if the behavior is guarded with a GUC.  I just
>>> >> don't understand what all the fuss is about.  The default behavior of
>>> >> logging that is well established by other languages (for example java)
>>> >> that manage error stack for you should be to:
>>> >>
>>> >> *) Give stack trace when an uncaught exception is thrown
>>> >> *) Do not give stack trace in all other logging cases unless asked for
>>> >
>>> > Java's exception handling is so different from PostgreSQL's errors that
>>> > I
>>> > don't think there's much to be learned from that. But I'll bite:
>>> >
>>> > First of all, Java's exceptions always contain a stack trace. It's up
>>> > to you
>>> > when you catch an exception to decide whether to print it or not. "try
>>> > { ...
>>> > } catch (Exception e) { e.printStackTrace() }" is fairly common,
>>> > actually.
>>> > There is nothing like a NOTICE in Java, i.e. an exception that's thrown
>>> > but
>>> > doesn't affect the control flow. The best I can think of is
>>> > System.out.println(), which of course has no stack trace attached to
>>> > it.
>>>
>>> exactly.
>>>
>>> > Perhaps you're arguing that NOTICE is more like printing to stderr, and
>>> > should never contain any context information. I don't think that would
>>> > be an
>>> > improvement. It's very handy to have the context information available
>>> > if
>>> > don't know where a NOTICE is coming from, even if in most cases you're
>>> > not
>>> > interested in it.
>>>
>>> That's exactly what I'm arguing.  NOTICE (and WARNING) are for
>>> printing out information to client side logging; it's really the only
>>> tool we have for that purpose and it fits that role perfectly.  Of
>>> course, you may want to have NOTICE print context, especially when
>>> debugging, but some control over that would be nice and in most cases
>>> it's really not necessary.  I really don't understand the objection to
>>> offering control over that behavior although I certainly understand
>>> wanting to keep the default behavior as it currently is.
>>>
>>> > This is really quite different from a programming language's exception
>>> > handling. First, there's a server, which produces the errors, and a
>>> > separate
>>> > client, which displays them. You cannot catch an exception in the
>>> > client.
>>> >
>>> > BTW, let me throw in one use case to consider. We've been talking about
>>> > psql, and what to print, but imagine a more sophisticated client like
>>> > pgAdmin. It's not limited to either printing the context or not. It
>>> > could
>>> > e.g. hide the context information of all messages when they occur, but
>>> > if
>>> > you double-click on it, it's expanded to show all the context, location
>>> > and
>>> > all. You can't do that if the server doesn't send the context
>>> > information in
>>> > the first place.
>>> >
>>> >> I would be happy to show you the psql redirected output logs from my
>>> >> nightly server processes that spew into the megabytes because of
>>> >> logging various high level steps (did this, did that).
>>> >
>>> > Oh, I believe you. I understand what the problem is, we're only talking
>>> > about how best to address it.
>>>
>>> Yeah.  For posterity, a psql based solution would work fine for me,
>>> but a server side solution has a lot of advantages (less protocol
>>> chatter, more configurability, keeping libpq/psql light).
>>
>>
>> After some work on reduced version of "plpgsql.min_context" patch I am
>> inclining to think so ideal solution needs more steps - because this issue
>> has more than one dimension.
>>
>> There are two independent issues:
>>
>> 1. old plpgsql workaround that reduced the unwanted call stack info for
>> RAISE NOTICE. Negative side effect of this workaround is missing context
>> info about the RAISE command that raises the exception. We know a function,
>> but we don't know a line of related RAISE statement. The important is fact,
>> so NOTICE doesn't bubble to up. So this workaround was relative successful
>> without to implement some filtering on client or log side.
>
>
> I found a other issue of this workaround - it doesn't work well for nested
> SQL statement call, when inner statement invoke RAISE NOTICE. In this case a
> context is showed too.
>
> postgres=# insert into xx values(60);
> NOTICE:  <<<<<>>>>>>
> NOTICE:  trigger_func(before_ins_stmt) called: action = INSERT, when =
> BEFORE, level = STATEMENT
> CONTEXT:  SQL statement "INSERT INTO boo VALUES(30)"
> PL/pgSQL function hh() line 1 at SQL statement
>
> So filtering context for selected environment is not good idea. The result
> is fragmented context, where is not clear, what is missing.

not quite following you. Is this a problem with your proposed patch,
or a reason why your patch is the 'right' implementation?

merlin

On Wed, Jul 8, 2015 at 11:28 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Hi
>
> second version of this patch
>
> make check-world passed

quickly scanning the patch, the implementation is trivial (minus
regression test adjustments), and is, IMSNSHO, the right solution.

Several of the source level comments need some minor wordsmithing and
the GUCs are missing documentation.  If we've got consensus on the
approach, I'll pitch in on that.

merlin

On Thu, Jul 9, 2015 at 10:48 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> 2015-07-09 15:17 GMT+02:00 Merlin Moncure <mmoncure@gmail.com>:
>>
>> On Wed, Jul 8, 2015 at 11:28 PM, Pavel Stehule <pavel.stehule@gmail.com>
>> wrote:
>> > Hi
>> >
>> > second version of this patch
>> >
>> > make check-world passed
>>
>> quickly scanning the patch, the implementation is trivial (minus
>> regression test adjustments), and is, IMSNSHO, the right solution.
>
>
> yes, it is right way - the behave of RAISE statement will be much more
> cleaner
>>
>>
>> Several of the source level comments need some minor wordsmithing and
>> the GUCs are missing documentation.  If we've got consensus on the
>> approach, I'll pitch in on that.
>
> thank you

revised patch attached. added GUC docs and cleaned up pg_settings
language.  Also tested patch and it works beautifully.

Note, Pavel's patch does adjust default behavior to what we think is
the "right" settings.

merlin

On Thu, Jul 9, 2015 at 3:31 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
> 2015-07-09 20:08 GMT+02:00 Merlin Moncure <mmoncure@gmail.com>:
>>
>> On Thu, Jul 9, 2015 at 10:48 AM, Pavel Stehule <pavel.stehule@gmail.com>
>> wrote:
>> >
>> > 2015-07-09 15:17 GMT+02:00 Merlin Moncure <mmoncure@gmail.com>:
>> >>
>> >> On Wed, Jul 8, 2015 at 11:28 PM, Pavel Stehule
>> >> <pavel.stehule@gmail.com>
>> >> wrote:
>> >> > Hi
>> >> >
>> >> > second version of this patch
>> >> >
>> >> > make check-world passed
>> >>
>> >> quickly scanning the patch, the implementation is trivial (minus
>> >> regression test adjustments), and is, IMSNSHO, the right solution.
>> >
>> >
>> > yes, it is right way - the behave of RAISE statement will be much more
>> > cleaner
>> >>
>> >>
>> >> Several of the source level comments need some minor wordsmithing and
>> >> the GUCs are missing documentation.  If we've got consensus on the
>> >> approach, I'll pitch in on that.
>> >
>> > thank you
>>
>> revised patch attached. added GUC docs and cleaned up pg_settings
>> language.  Also tested patch and it works beautifully.
>>
>> Note, Pavel's patch does adjust default behavior to what we think is
>> the "right" settings.
>
>
> Thank you for documentation.
>
> There is small error - default for client_min_context is error - not notice.
> With this level a diff from regress tests is minimal. Default for
> log_min_context should be warning.

whoop!  thanks.   Also, I was playing a bit with the idea of making
client_min_context "superuser only" setting.  The idea being this
could be used to prevent leakage of stored procedure code in cases
where the admins don't want it to be exposed.  I figured it was a bad
idea though; it would frustrate debugging in reasonable cases.

merlin

On 07/21/2015 10:38 AM, Pavel Stehule wrote:
> where we are with this patch? Can I do some for it?

I still feel this approach is misguided, and we should be tweaking psql 
and/or libpq instead. I don't feel strongly though, and if some other 
committer wants to pick this up in its current form, I won't object. So 
this patch has reached an impasse, and if no-one else wants to pick this 
up, I'm going to mark this as "Returned with Feedback" and move on.
- Heikki

On Tue, Jul 21, 2015 at 2:53 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 07/21/2015 10:38 AM, Pavel Stehule wrote:
>>
>> where we are with this patch? Can I do some for it?
>
>
> I still feel this approach is misguided, and we should be tweaking psql
> and/or libpq instead. I don't feel strongly though, and if some other
> committer wants to pick this up in its current form, I won't object. So this
> patch has reached an impasse, and if no-one else wants to pick this up, I'm
> going to mark this as "Returned with Feedback" and move on.

That's unfortunate.  Maybe I'm missing something:

What does a client side implementation offer that a server side
implementation does not offer?

merlin

On 07/26/2015 08:34 AM, Pavel Stehule wrote:
> Hi
>
> here is complete patch, that introduce context filtering on client side.
> The core of this patch is trivial and small - almost all of size are
> trivial changes in regress tests - removing useless context.
>
> Documentation, check-world

Looks good to me at first glance. I'll mark this as Ready for Committer.

- Heikki

On 8/12/15 9:36 AM, Pavel Stehule wrote:
> So, there is common agreement on this version.

There are several instances of double semicolons.  Also, 
PsqlSettings.show_context doesn't look like a boolean to me.  For 
SHOW_CONTEXT, it would be good if the documentation mentioned the 
default value.

I'm somewhat worried that this is hiding important context from some 
NOTICE or WARNING messages intended for novice users, but probably not 
worried enough to go through all of them.  +3/8 from me, I guess.


.m

Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2015-08-12 11:07 GMT+02:00 Marko Tiikkaja <marko@joh.to>:
>> I'm somewhat worried that this is hiding important context from some
>> NOTICE or WARNING messages intended for novice users, but probably not
>> worried enough to go through all of them.  +3/8 from me, I guess.

> I fixed mentioned issues.

Okay, so to summarize where we seem to have ended up:

1. Remove the wart in plpgsql that causes it to suppress the innermost
CONTEXT line for RAISE.  (I think pretty much everyone agrees that this
*is* a wart.  The question is how to get rid of it without a decrease
in usability for simple cases.)

2. Change psql so that by default, it hides the entire CONTEXT output
for messages that are of less than ERROR severity.  Add a new magic
\set variable that allows choosing this behavior, or display CONTEXT
always, or display CONTEXT never.

3. Since psql actually uses libpq for formatting server messages,
add an API to libpq that implements these CONTEXT hide/show options.

The actual code changes are pretty small, but there's rather a large
change in regression test outputs; which is unsurprising, because this
heuristic for what's of interest is entirely different from the old one.

Is everyone satisfied with this solution?  It's okay with me, though
I'm concerned that there will be complaints about loss of backwards
compatibility.  (It's hard to see how the contents of CONTEXT error
fields would be a big application compatibility issue, but you never
know.)

If there are not major objections, I'll work on cleaning up and
committing the patch.  There is still work needed (eg, the API addition
of point 3 is undocumented), but the main question is just whether we
are happy with making things work this way.
        regards, tom lane

I wrote:
> If there are not major objections, I'll work on cleaning up and
> committing the patch.

Pushed.  I'm not too sure about the expected outputs for python other
than 2.6, nor for sepgsql, but hopefully the buildfarm will provide
feedback.

BTW, I noticed that the PageOutput line counts for psql's usage(),
slashUsage(), and helpVariables() were all three wrong, which I'm afraid
has been their usual state in the past too.  Since commit 07c8651dd91d5aea
there's been a pretty easy way to check them, which I added comments
about; but I don't hold much hope that that will fix anything.  I wonder
whether there's some way to not need to maintain those counts manually.
        regards, tom lane

On 09/05/2015 09:05 AM, Tom Lane wrote:
> I wrote:
>> If there are not major objections, I'll work on cleaning up and
>> committing the patch.
>
> Pushed.  I'm not too sure about the expected outputs for python other
> than 2.6, nor for sepgsql, but hopefully the buildfarm will provide
> feedback.

We don't have the buildfarm actually checking sepgsql yet, but I'll
check it out manually today or tomorrow.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

I wrote:
> BTW, I noticed that the PageOutput line counts for psql's usage(),
> slashUsage(), and helpVariables() were all three wrong, which I'm afraid
> has been their usual state in the past too.  Since commit 07c8651dd91d5aea
> there's been a pretty easy way to check them, which I added comments
> about; but I don't hold much hope that that will fix anything.  I wonder
> whether there's some way to not need to maintain those counts manually.

I seem to recall past proposals to fix that by putting the lines into
static char * arrays, which foundered on the fact that the output's not
necessarily constant.  But it suddenly strikes me that there's an easy
fix.  We can generate the output into a PQExpBuffer, which is just as
flexible as fprintf() is today, then count the lines and finally print.

Ordinarily I might think that was overkill, but given the number of times
that we've failed to update those counts in the past, I think this is
definitely a worthwhile investment in maintainability.

Or we could just give up and replace the counts by INT_MAX, forcing use
of the pager unless you've turned it off.  All of those outputs are long
enough now that it's hard to believe there are any common screen layouts
where you don't end up invoking the pager anyway.  (usage() is 60 lines,
the others are more.)  This is probably the reason why we've seldom
noticed they're wrong --- it barely matters anymore.

One way or the other I think it's past time to get out of the business
of maintaining these counts.  I'm willing to do the work of using a
PQExpBuffer if people think it's worth the trouble to have an accurate
count, but it may not be worth the code space.

Thoughts?
        regards, tom lane

On Sat, Sep 5, 2015 at 5:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Ordinarily I might think that was overkill, but given the number of times
> that we've failed to update those counts in the past, I think this is
> definitely a worthwhile investment in maintainability.

So to preface this, this was just a silly hack that turned out to be
more effective and simpler to code than I expected. But I suspect it's
still just a silly idea and easier to do what you suggested. Also, it
seems we often get the count wrong on SQL output and elsewhere anyways
and I'm not sure we really want to make that a strict rule. Also, as
someone who doesn't really like the whole "sometimes you get a pager
sometimes you don't" thing and turns it off whenever he sees it I'm
not in the best place to judge how much work it's worth to get the
line count right.

But that said, here's a tricksy patch that triggers an assertion
failure if the expected_lines is wrong. I intended it to trigger in
the regression tests so it only checks if the pager is actually off.
It wouldn't be hard to make it always check though.

--
greg

Greg Stark <stark@mit.edu> writes:
> But that said, here's a tricksy patch that triggers an assertion
> failure if the expected_lines is wrong. I intended it to trigger in
> the regression tests so it only checks if the pager is actually off.
> It wouldn't be hard to make it always check though.

Hmm ... that would put a premium on the linecount always being exactly
right (for all callers, not just the help functions).  Not sure that
I want to buy into that --- it would require more complexity in the
callers than is there now, for sure.
        regards, tom lane

On 09/05/2015 09:14 AM, Joe Conway wrote:
> On 09/05/2015 09:05 AM, Tom Lane wrote:
>> I wrote:
>>> If there are not major objections, I'll work on cleaning up and
>>> committing the patch.
>>
>> Pushed.  I'm not too sure about the expected outputs for python other
>> than 2.6, nor for sepgsql, but hopefully the buildfarm will provide
>> feedback.
>
> We don't have the buildfarm actually checking sepgsql yet, but I'll
> check it out manually today or tomorrow.


One-liner required for sepgsql -- attached committed and pushed.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Joe Conway <mail@joeconway.com> writes:
>> On 09/05/2015 09:05 AM, Tom Lane wrote:
>>> Pushed.  I'm not too sure about the expected outputs for python other
>>> than 2.6, nor for sepgsql, but hopefully the buildfarm will provide
>>> feedback.

> One-liner required for sepgsql -- attached committed and pushed.

Thanks for checking!
        regards, tom lane

On 9/5/15 3:50 PM, Tom Lane wrote:
> Greg Stark <stark@mit.edu> writes:
>> But that said, here's a tricksy patch that triggers an assertion
>> failure if the expected_lines is wrong. I intended it to trigger in
>> the regression tests so it only checks if the pager is actually off.
>> It wouldn't be hard to make it always check though.
>
> Hmm ... that would put a premium on the linecount always being exactly
> right (for all callers, not just the help functions).  Not sure that
> I want to buy into that --- it would require more complexity in the
> callers than is there now, for sure.

But only in an assert-enabled build. Surely there's enough other 
performance hits with asserts enabled that this wouldn't matter?

As for paging, ISTM the only people that would care are those with 
enormous terminal sizes would care, and assuming their pager is less 
simply adding -F to $LESS would get them their old behavior. So I think 
it's safe to just force paging.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

Jim Nasby <Jim.Nasby@bluetreble.com> writes:
> On 9/5/15 3:50 PM, Tom Lane wrote:
>> Greg Stark <stark@mit.edu> writes:
>>> But that said, here's a tricksy patch that triggers an assertion
>>> failure if the expected_lines is wrong.

>> Hmm ... that would put a premium on the linecount always being exactly
>> right (for all callers, not just the help functions).  Not sure that
>> I want to buy into that --- it would require more complexity in the
>> callers than is there now, for sure.

> But only in an assert-enabled build. Surely there's enough other 
> performance hits with asserts enabled that this wouldn't matter?

It's not about performance, it's about code complexity and maintenance
overhead.  I'm looking to *reduce* the amount of personpower expended
on those line counts, not increase it; but adding an assertion requirement
that the counts be exactly right would require us to spend more development
effort on making them right.

> As for paging, ISTM the only people that would care are those with 
> enormous terminal sizes would care, and assuming their pager is less 
> simply adding -F to $LESS would get them their old behavior. So I think 
> it's safe to just force paging.

Yeah, I'm leaning to just changing the counts to INT_MAX and being
done with it.
        regards, tom lane

On 09/05/2015 12:55 PM, Tom Lane wrote:
> I wrote:
>> BTW, I noticed that the PageOutput line counts for psql's usage(),
>> slashUsage(), and helpVariables() were all three wrong, which I'm afraid
>> has been their usual state in the past too.  Since commit 07c8651dd91d5aea
>> there's been a pretty easy way to check them, which I added comments
>> about; but I don't hold much hope that that will fix anything.  I wonder
>> whether there's some way to not need to maintain those counts manually.
> I seem to recall past proposals to fix that by putting the lines into
> static char * arrays, which foundered on the fact that the output's not
> necessarily constant.  But it suddenly strikes me that there's an easy
> fix.  We can generate the output into a PQExpBuffer, which is just as
> flexible as fprintf() is today, then count the lines and finally print.
>
> Ordinarily I might think that was overkill, but given the number of times
> that we've failed to update those counts in the past, I think this is
> definitely a worthwhile investment in maintainability.
>
> Or we could just give up and replace the counts by INT_MAX, forcing use
> of the pager unless you've turned it off.  All of those outputs are long
> enough now that it's hard to believe there are any common screen layouts
> where you don't end up invoking the pager anyway.  (usage() is 60 lines,
> the others are more.)  This is probably the reason why we've seldom
> noticed they're wrong --- it barely matters anymore.
>
> One way or the other I think it's past time to get out of the business
> of maintaining these counts.  I'm willing to do the work of using a
> PQExpBuffer if people think it's worth the trouble to have an accurate
> count, but it may not be worth the code space.
>
> Thoughts?
>
>             


I'm not terribly happy about the INT_MAX idea. Counting lines in a 
PGExpBuffer seems OK. That way we could honor pager_min_lines, I hope.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:
> On 09/05/2015 12:55 PM, Tom Lane wrote:
>> Or we could just give up and replace the counts by INT_MAX, forcing use
>> of the pager unless you've turned it off.  All of those outputs are long
>> enough now that it's hard to believe there are any common screen layouts
>> where you don't end up invoking the pager anyway.  (usage() is 60 lines,
>> the others are more.)  This is probably the reason why we've seldom
>> noticed they're wrong --- it barely matters anymore.
>> 
>> One way or the other I think it's past time to get out of the business
>> of maintaining these counts.  I'm willing to do the work of using a
>> PQExpBuffer if people think it's worth the trouble to have an accurate
>> count, but it may not be worth the code space.

> I'm not terribly happy about the INT_MAX idea. Counting lines in a 
> PGExpBuffer seems OK. That way we could honor pager_min_lines, I hope.

TBH, I'm not detecting enough concern about this issue to make it worth
doing more than replacing the counts by INT_MAX.  Nobody has stepped up
and said "yeah, my terminal window is 100 lines high and I'll be really
annoyed if \? invokes the pager unnecessarily".  I plan to just do the
three-line fix and move on.
        regards, tom lane

On 09/09/2015 10:27 AM, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 09/05/2015 12:55 PM, Tom Lane wrote:
>>> Or we could just give up and replace the counts by INT_MAX, forcing use
>>> of the pager unless you've turned it off.  All of those outputs are long
>>> enough now that it's hard to believe there are any common screen layouts
>>> where you don't end up invoking the pager anyway.  (usage() is 60 lines,
>>> the others are more.)  This is probably the reason why we've seldom
>>> noticed they're wrong --- it barely matters anymore.
>>>
>>> One way or the other I think it's past time to get out of the business
>>> of maintaining these counts.  I'm willing to do the work of using a
>>> PQExpBuffer if people think it's worth the trouble to have an accurate
>>> count, but it may not be worth the code space.
>> I'm not terribly happy about the INT_MAX idea. Counting lines in a
>> PGExpBuffer seems OK. That way we could honor pager_min_lines, I hope.
> TBH, I'm not detecting enough concern about this issue to make it worth
> doing more than replacing the counts by INT_MAX.  Nobody has stepped up
> and said "yeah, my terminal window is 100 lines high and I'll be really
> annoyed if \? invokes the pager unnecessarily".  I plan to just do the
> three-line fix and move on.
>
>             


Do people really use terminals without scrollbars for serious work any 
more? Personally I'm in favor of forcing the pager less, not more. But 
I'm not going to make a fuss, I'm just surprised.

cheers

andrew