Re: 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

2013/8/21 Marko Tiikkaja <marko(at)joh(dot)to>

> 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.
>

+1

Pavel

>
> Thoughts?
>
>
> Regards,
> Marko Tiikkaja
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/**mailpref/pgsql-hackers<http://www.postgresql.org/mailpref/pgsql-hackers>
>

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(at)joh(dot)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(at)joh(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Marko Tiikkaja <marko(at)joh(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Marko Tiikkaja <marko(at)joh(dot)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(at)joh(dot)to> wrote:
> On 8/21/13 5:05 PM, Merlin Moncure wrote:
>>
>> On Wed, Aug 21, 2013 at 9:22 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>
>>> Marko Tiikkaja <marko(at)joh(dot)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

2013/8/21 Merlin Moncure <mmoncure(at)gmail(dot)com>

> On Wed, Aug 21, 2013 at 10:07 AM, Marko Tiikkaja <marko(at)joh(dot)to> wrote:
> > On 8/21/13 5:05 PM, Merlin Moncure wrote:
> >>
> >> On Wed, Aug 21, 2013 at 9:22 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >>>
> >>> Marko Tiikkaja <marko(at)joh(dot)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'
>
>
it is not bad idea

Pavel

> merlin
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

On 2013-08-21 17:18, Merlin Moncure wrote:
> On Wed, Aug 21, 2013 at 10:07 AM, Marko Tiikkaja <marko(at)joh(dot)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(at)gmail(dot)com> writes:
> On Wed, Aug 21, 2013 at 10:07 AM, Marko Tiikkaja <marko(at)joh(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Merlin Moncure <mmoncure(at)gmail(dot)com> writes:
>> On Wed, Aug 21, 2013 at 10:07 AM, Marko Tiikkaja <marko(at)joh(dot)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

Hello

I played with this topic little bit

If I understand, the main problem is in console (or pgAdmin) output.

create or replace function foo()
returns void as $$
begin
for i in 1..5
loop
raise notice '>>>>> *****';
end loop;
raise exception '***************';
end;
$$ language plpgsql;

postgres=# select foo();
NOTICE: >>>>> *****
NOTICE: >>>>> *****
NOTICE: >>>>> *****
NOTICE: >>>>> *****
NOTICE: >>>>> *****
ERROR: ***************
Time: 2.024 ms
postgres=# \set VER
VERBOSITY VERSION
postgres=# \set VERBOSITY

postgres=# \set VERBOSITY

postgres=# \set VERBOSITY terse
postgres=# select foo();
NOTICE: >>>>> *****
NOTICE: >>>>> *****
NOTICE: >>>>> *****
NOTICE: >>>>> *****
NOTICE: >>>>> *****
ERROR: ***************
Time: 0.908 ms
postgres=# \set VERBOSITY verbose
postgres=# select foo();
NOTICE: 00000: >>>>> *****
LOCATION: exec_stmt_raise, pl_exec.c:3051
NOTICE: 00000: >>>>> *****
LOCATION: exec_stmt_raise, pl_exec.c:3051
NOTICE: 00000: >>>>> *****
LOCATION: exec_stmt_raise, pl_exec.c:3051
NOTICE: 00000: >>>>> *****
LOCATION: exec_stmt_raise, pl_exec.c:3051
NOTICE: 00000: >>>>> *****
LOCATION: exec_stmt_raise, pl_exec.c:3051
ERROR: P0001: ***************
LOCATION: exec_stmt_raise, pl_exec.c:3051

Time: 0.314 ms

I see a two little bit not nice issues:

a) in terse mode missing a CONTEXT for RAISED error
b) in verbose mode missing a CONTEXT for messages, for error too, and
useless LOCATION is showed.

LOCATION is absolutely useless for custom messages.

so I removed a context filtering

postgres=# select foo();
NOTICE: >>>>> *****
CONTEXT: PL/pgSQL function foo() line 5 at RAISE
NOTICE: >>>>> *****
CONTEXT: PL/pgSQL function foo() line 5 at RAISE
NOTICE: >>>>> *****
CONTEXT: PL/pgSQL function foo() line 5 at RAISE
NOTICE: >>>>> *****
CONTEXT: PL/pgSQL function foo() line 5 at RAISE
NOTICE: >>>>> *****
CONTEXT: PL/pgSQL function foo() line 5 at RAISE
ERROR: ***************
CONTEXT: PL/pgSQL function foo() line 7 at RAISE
Time: 3.842 ms
postgres=# \set VERBOSITY verbose
postgres=# select foo();
NOTICE: 00000: >>>>> *****
CONTEXT: PL/pgSQL function foo() line 5 at RAISE
LOCATION: exec_stmt_raise, pl_exec.c:3046
NOTICE: 00000: >>>>> *****
CONTEXT: PL/pgSQL function foo() line 5 at RAISE
LOCATION: exec_stmt_raise, pl_exec.c:3046
NOTICE: 00000: >>>>> *****
CONTEXT: PL/pgSQL function foo() line 5 at RAISE
LOCATION: exec_stmt_raise, pl_exec.c:3046
NOTICE: 00000: >>>>> *****
CONTEXT: PL/pgSQL function foo() line 5 at RAISE
LOCATION: exec_stmt_raise, pl_exec.c:3046
NOTICE: 00000: >>>>> *****
CONTEXT: PL/pgSQL function foo() line 5 at RAISE
LOCATION: exec_stmt_raise, pl_exec.c:3046
ERROR: P0001: ***************
CONTEXT: PL/pgSQL function foo() line 7 at RAISE
LOCATION: exec_stmt_raise, pl_exec.c:3046
Time: 0.761 ms

We should not see a CONTEXT for DEFAULT verbosity and NOTICE level, after
little bit change I got a satisfied output

postgres=# select foo();
>>>NOTICE: >>>>> *****
>>>NOTICE: >>>>> *****
>>>NOTICE: >>>>> *****
>>>NOTICE: >>>>> *****
>>>NOTICE: >>>>> *****
ERROR: ***************
CONTEXT: PL/pgSQL function foo() line 7 at RAISE
Time: 2.434 ms
postgres=# \set VERBOSITY verbose
postgres=# select foo();
>>>NOTICE: 00000: >>>>> *****
CONTEXT: PL/pgSQL function foo() line 5 at RAISE
LOCATION: exec_stmt_raise, pl_exec.c:3046
>>>NOTICE: 00000: >>>>> *****
CONTEXT: PL/pgSQL function foo() line 5 at RAISE
LOCATION: exec_stmt_raise, pl_exec.c:3046
>>>NOTICE: 00000: >>>>> *****
CONTEXT: PL/pgSQL function foo() line 5 at RAISE
LOCATION: exec_stmt_raise, pl_exec.c:3046
>>>NOTICE: 00000: >>>>> *****
CONTEXT: PL/pgSQL function foo() line 5 at RAISE
LOCATION: exec_stmt_raise, pl_exec.c:3046
>>>NOTICE: 00000: >>>>> *****
CONTEXT: PL/pgSQL function foo() line 5 at RAISE
LOCATION: exec_stmt_raise, pl_exec.c:3046
ERROR: P0001: ***************
CONTEXT: PL/pgSQL function foo() line 7 at RAISE
LOCATION: exec_stmt_raise, pl_exec.c:3046
Time: 0.594 ms

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?

Regards

Pavel

On Thu, Aug 22, 2013 at 2:08 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

2013/8/22 Marko Tiikkaja <marko(at)joh(dot)to>

> 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.
>
>

word "DEBUG" is not good - it is used for Postgres debugging as log level

Pavel

>
> Regards,
> Marko Tiikkaja
>
>

2013/8/22 Marko Tiikkaja <marko(at)joh(dot)to>

> 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.
>

do you prepare patch ?

Pavel

>
>
> Regards,
> Marko Tiikkaja
>
>

On 8/23/13 8:38 AM, Pavel Stehule wrote:
> 2013/8/22 Marko Tiikkaja <marko(at)joh(dot)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

2013/8/23 Marko Tiikkaja <marko(at)joh(dot)to>

> On 8/23/13 8:38 AM, Pavel Stehule wrote:
>
> 2013/8/22 Marko Tiikkaja <marko(at)joh(dot)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.
>

The name is not important. What I would, for DEFAULT verbosity, to see a
context when RAISE EXCEPTION is used. It is a bug now, I think

Regards

Pavel

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

2013/9/14 Marko Tiikkaja <marko(at)joh(dot)to>

> 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?
>
>
+1

Regards

Pavel

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

Hi Marko,

I have reviewed this patch.

1. Patch applies well.
2. make and make install is fine
3. make check is fine too.

But as Peter pointed out plperl regression tests are failing.

I just did grep on .sql files and found following files which has RAISE
statement into it. These files too need expected output changes. Please run
these testcases to get diffs.

./src/pl/plperl/sql/plperl_elog.sql
./src/pl/plpython/sql/plpython_error.sql
./src/pl/plpython/sql/plpython_setof.sql
./src/pl/plpython/sql/plpython_quote.sql
./contrib/sepgsql/sql/label.sql
./contrib/sepgsql/sql/ddl.sql

Code changes looks fine to me.

Thanks

--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation

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

2015-01-22 12:37 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)to>:

> 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.
>

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.

Regards

Pavel

>
>
> .marko
>

On 1/22/15 6:03 PM, Pavel Stehule wrote:
> 2015-01-22 12:37 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)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

2015-01-26 13:02 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)to>:

> On 1/22/15 6:03 PM, Pavel Stehule wrote:
>
>> 2015-01-22 12:37 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)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.
>

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 - but on second
hand current behave is not critical too - we can wait.

>
>
> .marko
>

On 1/26/15 1:14 PM, Pavel Stehule wrote:
> 2015-01-26 13:02 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)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

2015-01-26 13:39 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)to>:

> On 1/26/15 1:14 PM, Pavel Stehule wrote:
>
>> 2015-01-26 13:02 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)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?
>

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

>
> - 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.
>

if it is necessary, then we can modify libpq

Regards

Pavel

>
>
> .marko
>

On 1/26/15 1:44 PM, Pavel Stehule wrote:
> 2015-01-26 13:39 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)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

2015-01-26 14:02 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)to>:

> On 1/26/15 1:44 PM, Pavel Stehule wrote:
>
>> 2015-01-26 13:39 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)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.
>

I had to though little bit more - and I am thinking so we should to return
back to start of this thread.

Current state:

1. RAISE in plpgsql doesn't show a context - what we want in RAISE NOTICE
and we don't want in RAISE EXCEPTION

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.

2. Personally I prefer a little bit conceptual solution, that needs a libpq
change because I wish some mode between terse and verbose mode - I would
not to see context for NOTICEs, but I would to see context for errors. This
request is generic - independent on used PL. @2 is my feature request and
it is possible independent on @1.

3. your proposal plpgsql.suppress_simple_error_context fix the @2 partially
- just I prefer a generic solution that will be available for all PL -
exception processing is same for all PL, so filtering should be common too.

Regards

Pavel

>
>
> .m
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2015-01-26 14:02 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)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

2015-01-26 16:14 GMT+01:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> > 2015-01-26 14:02 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)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 don't understand. There can be a overhead due useless transformation some
data to client side. But all what it need - errcontext and errlevel is
possible.

> 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.
>

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.

Cannot be solution?

estate->err_text = stmt->elog_level == ERROR ? estate->err_text :
raise_skip_msg ;

Regards

Pavel

>
> 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 extra coding 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

2015-01-26 16:46 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
> 2015-01-26 16:14 GMT+01:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>
>> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> > 2015-01-26 14:02 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)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 don't understand. There can be a overhead due useless transformation
> some data to client side. But all what it need - errcontext and errlevel is
> possible.
>
>
>> 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.
>>
>
> 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.
>
> Cannot be solution?
>

I would to wakeup this thread.

>
> 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.

Regards

Pavel

>
> Regards
>
> Pavel
>
>
>
>
>>
>> regards, tom lane
>>
>
>

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

2015-04-23 9:53 GMT+02:00 Marko Tiikkaja <marko(at)joh(dot)to>:

> 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.
>

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.

I am not to against to any special option to RAISE statement. Have you some
idea?

What about a enhancing a USING clause?

example:

RAISE NOTICE USING message = '', with_context = true
RAISE EXCEPTION USING message = '', with_context = false

Regards

Pavel

>
>
> .m
>

On Thu, Apr 23, 2015 at 4:56 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

2015-04-23 15:47 GMT+02:00 Robert Haas <robertmhaas(at)gmail(dot)com>:

> On Thu, Apr 23, 2015 at 4:56 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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?

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

On Thu, Apr 23, 2015 at 9:55 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> On Thu, Apr 23, 2015 at 4:56 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

2015-04-23 16:12 GMT+02:00 Robert Haas <robertmhaas(at)gmail(dot)com>:

> On Thu, Apr 23, 2015 at 9:55 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >> On Thu, Apr 23, 2015 at 4:56 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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.
>

I am not sure, if explained it well. I would to emit context for ERROR and
higher by default. And I would not to emit context for any less than ERROR
by default (I am not sure about WARNING level).

But it can be changed by some option in RAISE statement like Marko proposes
- possible to change by GUC globally, because it doesn't change a behave of
application.

For current behave I have a problem with ERROR level in plpgsql where the
context is missing now. On second hand I am thinking so current behave is
ok for NOTICE level .

I am not against to any new option in RAISE statement.

If there is some collision between me and Marko, then it is in opinion what
have to be default behave for NOTICE level. I strongly prefer don't show
context there. But I can accept some global switch too.

Regards

Pavel

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

2015-04-24 12:11 GMT+02:00 Joel Jacobson <joel(at)trustly(dot)com>:

> 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 proposal looks very practical - it can be very good start point - and
it doesn't block any next discuss about enhancing RAISE statement, what I
would to have too (bat can be separate issue). I like it.

Regards

Pavel

>
> 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(at)trustly(dot)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

2015-04-24 13:16 GMT+02:00 Robert Haas <robertmhaas(at)gmail(dot)com>:

> On Fri, Apr 24, 2015 at 6:11 AM, Joel Jacobson <joel(at)trustly(dot)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.
>

It can be PLpgSQL only GUC. Probably it is a most problematic case.

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

There is a agreement about it - but I am expecting a harder discussion
about what will be default, and the discussion about syntax should not be
simple too.

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

On Fri, Apr 24, 2015 at 1:16 PM, Robert Haas <robertmhaas(at)gmail(dot)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

2015-04-24 16:02 GMT+02:00 Joel Jacobson <joel(at)trustly(dot)com>:

> On Fri, Apr 24, 2015 at 1:16 PM, Robert Haas <robertmhaas(at)gmail(dot)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
>

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. :)

Regards

Pavel

On Fri, Apr 24, 2015 at 6:07 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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.

2015-04-24 19:16 GMT+02:00 Joel Jacobson <joel(at)trustly(dot)com>:

> On Fri, Apr 24, 2015 at 6:07 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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.
>

It looks like discussion KDE x GNOME.

GUC that has simply effect on behave without performance impact should not
be problem - like log_lock_wait, log_min_duration and similar. I am sure so
we would it.

The problematic GUC are a performance, planner, bgwriter, checkpoint
related.

Hi

2015-04-24 19:16 GMT+02:00 Joel Jacobson <joel(at)trustly(dot)com>:

> On Fri, Apr 24, 2015 at 6:07 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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.
>

I played with some prototype and I am thinking so we need only one GUC

plpgsql.display_context_messages = 'none'; -- compatible with current
plpgsql.display_context_messages = 'all';
plpgsql.display_context_messages = 'exception, log'; -- what I prefer

I implemented [ (WITH|WITHOUT) CONTEXT ] clause for RAISE statement

RAISE NOTICE WITH CONTEXT 'some message';
RAISE NOTICE WITH CONTEXT USING message = 'some message';
RAISE EXCEPTION WITHOUT CONTEXT 'other message';

The patch is very small with full functionality (without documentation) - I
am thinking so it can work. This patch is back compatible - and allow to
change default behave simply.

plpgsql.display_context_messages can be simplified to some like
plpgsql.display_context_min_messages

What do you think about it?

Regards

Pavel

+1

On Sat, Apr 25, 2015 at 10:23 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
wrote:

> Hi
>
> 2015-04-24 19:16 GMT+02:00 Joel Jacobson <joel(at)trustly(dot)com>:
>
>> On Fri, Apr 24, 2015 at 6:07 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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.
>>
>
> I played with some prototype and I am thinking so we need only one GUC
>
> plpgsql.display_context_messages = 'none'; -- compatible with current
> plpgsql.display_context_messages = 'all';
> plpgsql.display_context_messages = 'exception, log'; -- what I prefer
>
> I implemented [ (WITH|WITHOUT) CONTEXT ] clause for RAISE statement
>
> RAISE NOTICE WITH CONTEXT 'some message';
> RAISE NOTICE WITH CONTEXT USING message = 'some message';
> RAISE EXCEPTION WITHOUT CONTEXT 'other message';
>
> The patch is very small with full functionality (without documentation) -
> I am thinking so it can work. This patch is back compatible - and allow to
> change default behave simply.
>
> plpgsql.display_context_messages can be simplified to some like
> plpgsql.display_context_min_messages
>
> What do you think about it?
>
> Regards
>
> Pavel
>
>

Hi

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.

Documentation added.

Regards

Pavel

2015-04-25 22:23 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

> Hi
>
> 2015-04-24 19:16 GMT+02:00 Joel Jacobson <joel(at)trustly(dot)com>:
>
>> On Fri, Apr 24, 2015 at 6:07 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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.
>>
>
> I played with some prototype and I am thinking so we need only one GUC
>
> plpgsql.display_context_messages = 'none'; -- compatible with current
> plpgsql.display_context_messages = 'all';
> plpgsql.display_context_messages = 'exception, log'; -- what I prefer
>
> I implemented [ (WITH|WITHOUT) CONTEXT ] clause for RAISE statement
>
> RAISE NOTICE WITH CONTEXT 'some message';
> RAISE NOTICE WITH CONTEXT USING message = 'some message';
> RAISE EXCEPTION WITHOUT CONTEXT 'other message';
>
> The patch is very small with full functionality (without documentation) -
> I am thinking so it can work. This patch is back compatible - and allow to
> change default behave simply.
>
> plpgsql.display_context_messages can be simplified to some like
> plpgsql.display_context_min_messages
>
> What do you think about it?
>
> Regards
>
> Pavel
>
>

Looks good Pavel!

May I just suggest you add the default case
to src/test/regress/sql/plpgsql.sql
and src/test/regress/expected/plpgsql.out, to make it easier for the
reviewer to compare the difference between what happens in the default
case, when not using the raise-syntax and not using the GUCs?

Suggested addition to the beginning of src/test/regress/sql/plpgsql.sql:
+do $$
+begin
+ raise notice 'hello';
+end;
+$$;
+
+do $$
+begin
+ raise exception 'hello';
+end;
+$$;

Many thanks for this patch! I will pray to the PL/pgSQL God it will be
accepted. :)

Best regards,

Joel

On Sun, Apr 26, 2015 at 9:19 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
wrote:

> Hi
>
> 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.
>
> Documentation added.
>
> Regards
>
> Pavel
>
> 2015-04-25 22:23 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>
>> Hi
>>
>> 2015-04-24 19:16 GMT+02:00 Joel Jacobson <joel(at)trustly(dot)com>:
>>
>>> On Fri, Apr 24, 2015 at 6:07 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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.
>>>
>>
>> I played with some prototype and I am thinking so we need only one GUC
>>
>> plpgsql.display_context_messages = 'none'; -- compatible with current
>> plpgsql.display_context_messages = 'all';
>> plpgsql.display_context_messages = 'exception, log'; -- what I prefer
>>
>> I implemented [ (WITH|WITHOUT) CONTEXT ] clause for RAISE statement
>>
>> RAISE NOTICE WITH CONTEXT 'some message';
>> RAISE NOTICE WITH CONTEXT USING message = 'some message';
>> RAISE EXCEPTION WITHOUT CONTEXT 'other message';
>>
>> The patch is very small with full functionality (without documentation) -
>> I am thinking so it can work. This patch is back compatible - and allow to
>> change default behave simply.
>>
>> plpgsql.display_context_messages can be simplified to some like
>> plpgsql.display_context_min_messages
>>
>> What do you think about it?
>>
>> Regards
>>
>> Pavel
>>
>>
>

2015-04-27 16:05 GMT+02:00 Joel Jacobson <joel(at)trustly(dot)com>:

> Looks good Pavel!
>
> May I just suggest you add the default case
> to src/test/regress/sql/plpgsql.sql
> and src/test/regress/expected/plpgsql.out, to make it easier for the
> reviewer to compare the difference between what happens in the default
> case, when not using the raise-syntax and not using the GUCs?
>
> Suggested addition to the beginning of src/test/regress/sql/plpgsql.sql:
> +do $$
> +begin
> + raise notice 'hello';
> +end;
> +$$;
> +
> +do $$
> +begin
> + raise exception 'hello';
> +end;
> +$$;
>

done

>
> Many thanks for this patch! I will pray to the PL/pgSQL God it will be
> accepted. :)
>

:) -- please, do review, or fix documentation in this patch.

I hope, so it will be merged early in 9.6 cycle. It is relatively simple.

Pavel

>
> Best regards,
>
> Joel
>
>
> On Sun, Apr 26, 2015 at 9:19 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
>
>> Hi
>>
>> 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.
>>
>> Documentation added.
>>
>> Regards
>>
>> Pavel
>>
>> 2015-04-25 22:23 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>>
>>> Hi
>>>
>>> 2015-04-24 19:16 GMT+02:00 Joel Jacobson <joel(at)trustly(dot)com>:
>>>
>>>> On Fri, Apr 24, 2015 at 6:07 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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.
>>>>
>>>
>>> I played with some prototype and I am thinking so we need only one GUC
>>>
>>> plpgsql.display_context_messages = 'none'; -- compatible with current
>>> plpgsql.display_context_messages = 'all';
>>> plpgsql.display_context_messages = 'exception, log'; -- what I prefer
>>>
>>> I implemented [ (WITH|WITHOUT) CONTEXT ] clause for RAISE statement
>>>
>>> RAISE NOTICE WITH CONTEXT 'some message';
>>> RAISE NOTICE WITH CONTEXT USING message = 'some message';
>>> RAISE EXCEPTION WITHOUT CONTEXT 'other message';
>>>
>>> The patch is very small with full functionality (without documentation)
>>> - I am thinking so it can work. This patch is back compatible - and allow
>>> to change default behave simply.
>>>
>>> plpgsql.display_context_messages can be simplified to some like
>>> plpgsql.display_context_min_messages
>>>
>>> What do you think about it?
>>>
>>> Regards
>>>
>>> Pavel
>>>
>>>
>>
>

On Sun, Apr 26, 2015 at 4:19 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
wrote:
>
> Hi
>
> 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" ?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

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(dot)stehule(at)gmail(dot)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 Mon, Apr 27, 2015 at 6:14 PM, Marko Tiikkaja <marko(at)joh(dot)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"

On 4/27/15 11:47 AM, Joel Jacobson wrote:
> On Mon, Apr 27, 2015 at 6:14 PM, Marko Tiikkaja <marko(at)joh(dot)to
> <mailto:marko(at)joh(dot)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

2015-04-27 22:53 GMT+02:00 Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>:

> On 4/27/15 11:47 AM, Joel Jacobson wrote:
>
>> On Mon, Apr 27, 2015 at 6:14 PM, Marko Tiikkaja <marko(at)joh(dot)to
>> <mailto:marko(at)joh(dot)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?

it affect client and log together

maybe "min_context"

Pavel

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

2015-04-28 19:44 GMT+02:00 Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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

third variant with GUC plpgsql.min_context

Regards

Pavel

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

2015-04-30 10:24 GMT+02:00 Marko Tiikkaja <marko(at)joh(dot)to>:

> 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.
>

It doesn't look well - because it should be solve by errhidecontext(true)

yes, there is a issue in send_message_to_frontend - this ignore
edata->hide_ctx field. After fixing, it working as expected - so this is a
bug in implementation of errhidecontext()

should be

if (edata->context && !edata->hide_ctx)
{
pq_sendbyte(&msgbuf, PG_DIAG_CONTEXT);
err_sendstring(&msgbuf, edata->context);
}

and probably getting stack in err_finish should be fixed too:

if (!edata->hide_ctx)
for (econtext = error_context_stack;
econtext != NULL;
econtext = econtext->previous)
(*econtext->callback) (econtext->arg);

Regards

Pavel

I'll look on this issue.

Regards

Pavel

>
>
> .m
>

2015-04-30 10:50 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
> 2015-04-30 10:24 GMT+02:00 Marko Tiikkaja <marko(at)joh(dot)to>:
>
>> 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.
>>
>
> It doesn't look well - because it should be solve by errhidecontext(true)
>
> yes, there is a issue in send_message_to_frontend - this ignore
> edata->hide_ctx field. After fixing, it working as expected - so this is a
> bug in implementation of errhidecontext()
>
> should be
>
> if (edata->context && !edata->hide_ctx)
> {
> pq_sendbyte(&msgbuf, PG_DIAG_CONTEXT);
> err_sendstring(&msgbuf, edata->context);
> }
>
> and probably getting stack in err_finish should be fixed too:
>
> if (!edata->hide_ctx)
> for (econtext = error_context_stack;
> econtext != NULL;
> econtext = econtext->previous)
> (*econtext->callback) (econtext->arg);
>
> Regards
>
> Pavel
>
>
I am sending patch

>
>
> I'll look on this issue.
>
> Regards
>
> Pavel
>
>
>>
>>
>> .m
>>
>
>

On 01/26/2015 05:14 PM, Tom Lane wrote:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> 2015-01-26 14:02 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)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(at)iki(dot)fi> wrote:
> On 01/26/2015 05:14 PM, Tom Lane wrote:
>>
>> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>>>
>>> 2015-01-26 14:02 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)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

2015-07-07 15:56 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:

> On Tue, Jul 7, 2015 at 8:13 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
> wrote:
> > On 01/26/2015 05:14 PM, Tom Lane wrote:
> >>
> >> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> >>>
> >>> 2015-01-26 14:02 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)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
>

what is RAISE EXCEPTION - first or second case?

>
> 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(at)iki(dot)fi> wrote:
>> On 01/26/2015 05:14 PM, Tom Lane wrote:
>>>
>>> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>>>>
>>>> 2015-01-26 14:02 GMT+01:00 Marko Tiikkaja <marko(at)joh(dot)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(dot)stehule(at)gmail(dot)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(at)iki(dot)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

2015-07-07 18:15 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:

> On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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(at)iki(dot)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).
>

I prefer a server side solution too. With it I can have (as plpgsql
developer) bigger control of expected output.

Client can change this behave on global (min_context) or on language level
(plpgsql.min_context). If somebody afraid about security, we can to enforce
rule so min_context <= error always.

The possibility to enable or disable context per any RAISE statement is
nice to have, but it is not fundamental.

Other variant is a implementation of min_context on client side - but then
we cannot to ensure current behave and fix plpgsql raise exception issue
together.

Pavel

>
> merlin
>

2015-07-07 18:15 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:

> On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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(at)iki(dot)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

Regards

Pavel

> merlin
>

On Wed, Jul 8, 2015 at 1:35 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>
>
> 2015-07-07 18:15 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
>>
>> On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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(at)iki(dot)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

Hi

here is initial version of reduced patch. It is small code, but relative
big (although I expected bigger) change in tests.

if these changes are too big, then we have to introduce a plpgsql GUC
plpgsql.client_min_context and plpgsql.log_min_client. These GUC overwrite
global setting for plpgsql functions. I'll be more happy without these
variables. It decrease a impact of changes, but there is not clean what
behave is expected when PL are used together - and when fails PLpgSQL
function called from PLPerl. The context filtering should be really solved
on TOP level.

Regards

Pavel

2015-07-08 14:09 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:

> On Wed, Jul 8, 2015 at 1:35 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >
> >
> > 2015-07-07 18:15 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
> >>
> >> On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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(at)iki(dot)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
>

2015-07-08 8:35 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
> 2015-07-07 18:15 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
>
>> On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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(at)iki(dot)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.

Pavel

>
> 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
>
> Regards
>
> Pavel
>
>
>
>
>> merlin
>>
>
>

On Wed, Jul 8, 2015 at 4:39 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 2015-07-08 8:35 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>>
>>
>>
>> 2015-07-07 18:15 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
>>>
>>> On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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(at)iki(dot)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

2015-07-08 23:46 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:

> On Wed, Jul 8, 2015 at 4:39 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> > 2015-07-08 8:35 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> >>
> >>
> >>
> >> 2015-07-07 18:15 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
> >>>
> >>> On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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(at)iki(dot)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?
>

My patch fixes it. The current design is not consistent.

Pavel

>
> merlin
>

Hi

second version of this patch

make check-world passed

Regards

Pavel

2015-07-08 23:05 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

> Hi
>
> here is initial version of reduced patch. It is small code, but relative
> big (although I expected bigger) change in tests.
>
> if these changes are too big, then we have to introduce a plpgsql GUC
> plpgsql.client_min_context and plpgsql.log_min_client. These GUC overwrite
> global setting for plpgsql functions. I'll be more happy without these
> variables. It decrease a impact of changes, but there is not clean what
> behave is expected when PL are used together - and when fails PLpgSQL
> function called from PLPerl. The context filtering should be really solved
> on TOP level.
>
>
> Regards
>
> Pavel
>
> 2015-07-08 14:09 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
>
>> On Wed, Jul 8, 2015 at 1:35 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
>> wrote:
>> >
>> >
>> > 2015-07-07 18:15 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
>> >>
>> >> On Tue, Jul 7, 2015 at 9:04 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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(at)iki(dot)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 11:28 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

2015-07-09 15:17 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:

> On Wed, Jul 8, 2015 at 11:28 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

Pavel

>
> merlin
>

On Thu, Jul 9, 2015 at 10:48 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>
> 2015-07-09 15:17 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
>>
>> On Wed, Jul 8, 2015 at 11:28 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

2015-07-09 20:08 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:

> On Thu, Jul 9, 2015 at 10:48 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >
> > 2015-07-09 15:17 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
> >>
> >> On Wed, Jul 8, 2015 at 11:28 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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.

Regards

Pavel

>
> merlin
>

On Thu, Jul 9, 2015 at 3:31 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>
>
> 2015-07-09 20:08 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
>>
>> On Thu, Jul 9, 2015 at 10:48 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
>> wrote:
>> >
>> > 2015-07-09 15:17 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
>> >>
>> >> On Wed, Jul 8, 2015 at 11:28 PM, Pavel Stehule
>> >> <pavel(dot)stehule(at)gmail(dot)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

2015-07-09 22:57 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:

> On Thu, Jul 9, 2015 at 3:31 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >
> >
> > 2015-07-09 20:08 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
> >>
> >> On Thu, Jul 9, 2015 at 10:48 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com
> >
> >> wrote:
> >> >
> >> > 2015-07-09 15:17 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
> >> >>
> >> >> On Wed, Jul 8, 2015 at 11:28 PM, Pavel Stehule
> >> >> <pavel(dot)stehule(at)gmail(dot)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.
>

This is not designed for security usage. Probably there can be some rule in
future - the possibility to see or don't see a error context - OFF, ON.
For this reason, the setting a some min level is not good way.

Regards

Pavel

>
> merlin
>

2015-07-09 23:16 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
> 2015-07-09 22:57 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
>
>> On Thu, Jul 9, 2015 at 3:31 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
>> wrote:
>> >
>> >
>> > 2015-07-09 20:08 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
>> >>
>> >> On Thu, Jul 9, 2015 at 10:48 AM, Pavel Stehule <
>> pavel(dot)stehule(at)gmail(dot)com>
>> >> wrote:
>> >> >
>> >> > 2015-07-09 15:17 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
>> >> >>
>> >> >> On Wed, Jul 8, 2015 at 11:28 PM, Pavel Stehule
>> >> >> <pavel(dot)stehule(at)gmail(dot)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.
>>
>
> This is not designed for security usage. Probably there can be some rule
> in future - the possibility to see or don't see a error context - OFF, ON.
> For this reason, the setting a some min level is not good way.
>

Hi

where we are with this patch? Can I do some for it?

Regards

Pavel

>
> Regards
>
> Pavel
>
>
>
>
>>
>> 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

2015-07-21 9:53 GMT+02:00 Heikki Linnakangas <hlinnaka(at)iki(dot)fi>:

> 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.
>

Can we define, when we have a agreement and where not? The missing context
for RAISE EXCEPTION statement is a important issue and I would to solve it.

last patch has two parts:

1. remove plpgsql fix, that remove context for plpgsql RAISE statement - it
is working good enough for less NOTICE level, and work badly for EXCEPTION
and higher level.

2. enforce filtering of CONTEXT field on both sides (client/log)

For me, @1 is important and good solution (because there is strange
inconsistency between PLpgSQL and any other PL), @2 allows more ways - but
probably log_min_context (WARNING) is good idea too.

The advantage of context filtering on server side (client_min_message) is
one - it can be controlled by plpgsql - so I can do dynamic decision if
some NOTICE will have context or not.

The complexity will be +/- same for psql/libpq or for server side filtering.

Regards

Pavel

> - Heikki
>
>

On Tue, Jul 21, 2015 at 2:53 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)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

2015-07-21 16:58 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:

> On Tue, Jul 21, 2015 at 2:53 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)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?
>

I have not any problem to change the filtering to client side. Primary
question is fix of PLpgSQL RAISE statement issue - The context field
filtering is a necessary follow-up and trivial in both cases.

In this case, it is acceptable for all?

Regards

Pavel

>
> merlin
>

Hi

I am sending a next variant of filtering context patch.

postgres=# do $$ begin raise notice 'kuku'; end $$;
NOTICE: kuku
DO
Time: 2.441 ms
postgres=# do $$ begin raise exception 'kuku'; end $$;
ERROR: kuku
CONTEXT: PL/pgSQL function inline_code_block line 1 at RAISE
Time: 0.648 ms
postgres=# \set SHOW_CONTEXT always
postgres=# do $$ begin raise notice 'kuku'; end $$;
NOTICE: kuku
CONTEXT: PL/pgSQL function inline_code_block line 1 at RAISE
DO
Time: 0.702 ms

It is a variant, when I try to filter CONTEXT in libpq. There is little bit
less granularity on libpq side than server side, but still it is enough -
always, error, none.

This patch is without documentation, but basic regress tests works.

Regards

Pavel

2015-07-25 10:01 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
> 2015-07-21 16:58 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
>
>> On Tue, Jul 21, 2015 at 2:53 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)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?
>>
>
> I have not any problem to change the filtering to client side. Primary
> question is fix of PLpgSQL RAISE statement issue - The context field
> filtering is a necessary follow-up and trivial in both cases.
>
> In this case, it is acceptable for all?
>
> Regards
>
> Pavel
>
>
>>
>> merlin
>>
>
>

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

Regards

Pavel

2015-07-26 0:42 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

> Hi
>
> I am sending a next variant of filtering context patch.
>
> postgres=# do $$ begin raise notice 'kuku'; end $$;
> NOTICE: kuku
> DO
> Time: 2.441 ms
> postgres=# do $$ begin raise exception 'kuku'; end $$;
> ERROR: kuku
> CONTEXT: PL/pgSQL function inline_code_block line 1 at RAISE
> Time: 0.648 ms
> postgres=# \set SHOW_CONTEXT always
> postgres=# do $$ begin raise notice 'kuku'; end $$;
> NOTICE: kuku
> CONTEXT: PL/pgSQL function inline_code_block line 1 at RAISE
> DO
> Time: 0.702 ms
>
> It is a variant, when I try to filter CONTEXT in libpq. There is little
> bit less granularity on libpq side than server side, but still it is enough
> - always, error, none.
>
> This patch is without documentation, but basic regress tests works.
>
> Regards
>
> Pavel
>
>
>
> 2015-07-25 10:01 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>
>>
>>
>> 2015-07-21 16:58 GMT+02:00 Merlin Moncure <mmoncure(at)gmail(dot)com>:
>>
>>> On Tue, Jul 21, 2015 at 2:53 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)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?
>>>
>>
>> I have not any problem to change the filtering to client side. Primary
>> question is fix of PLpgSQL RAISE statement issue - The context field
>> filtering is a necessary follow-up and trivial in both cases.
>>
>> In this case, it is acceptable for all?
>>
>> Regards
>>
>> Pavel
>>
>>
>>>
>>> merlin
>>>
>>
>>
>