Re: bugfix: incomplete implementation of errhidecontext

Hi

current implementation of errhidecontext is not complete:

1. it sends context to client

2. it collect context although it will not be displayed

Attached patch fixing it

Pavel, will it be good if you separately submit the
"bugfix: incomplete implementation of errhidecontext"
patch in this commitfest?

2015-05-29 9:53 GMT+02:00 Jeevan Chalke <jeevan(dot)chalke(at)gmail(dot)com>:

> Pavel, will it be good if you separately submit the
> "bugfix: incomplete implementation of errhidecontext"
> patch in this commitfest?
>
>
ok, I'll do it

Pavel

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

Done

https://commitfest.postgresql.org/5/257/

2015-05-29 9:56 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
> 2015-05-29 9:53 GMT+02:00 Jeevan Chalke <jeevan(dot)chalke(at)gmail(dot)com>:
>
>> Pavel, will it be good if you separately submit the
>> "bugfix: incomplete implementation of errhidecontext"
>> patch in this commitfest?
>>
>>
> ok, I'll do it
>
> Pavel
>
>
>>
>> --
>> 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
>>
>
>

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

This is trivial bug fix in the area of hiding error context.

I observed that there are two places from which we are calling this function
to hide the context in log messages. Those were broken.

This patch fixes those.

So good to go in.

The new status of this patch is: Ready for Committer

On 2015-06-08 14:44:53 +0000, Jeevan Chalke wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world: tested, passed
> Implements feature: tested, passed
> Spec compliant: tested, passed
> Documentation: tested, passed
>
> This is trivial bug fix in the area of hiding error context.
>
> I observed that there are two places from which we are calling this function
> to hide the context in log messages. Those were broken.

Broken in which sense? They did prevent stuff to go from the server log?

I'm not convinced that hiding stuff from the client is really
necessarily the same as hiding it from the server log. We e.g. always
send the verbose log to the client, even if we only send the terse
version to the server log. I don't mind adjusting things for
errhidecontext(), but it's not "just a bug".

Greetings,

Andres Freund

2015-06-08 16:49 GMT+02:00 Andres Freund <andres(at)anarazel(dot)de>:

> On 2015-06-08 14:44:53 +0000, Jeevan Chalke wrote:
> > The following review has been posted through the commitfest application:
> > make installcheck-world: tested, passed
> > Implements feature: tested, passed
> > Spec compliant: tested, passed
> > Documentation: tested, passed
> >
> > This is trivial bug fix in the area of hiding error context.
> >
> > I observed that there are two places from which we are calling this
> function
> > to hide the context in log messages. Those were broken.
>
> Broken in which sense? They did prevent stuff to go from the server log?
>
> I'm not convinced that hiding stuff from the client is really
> necessarily the same as hiding it from the server log. We e.g. always
> send the verbose log to the client, even if we only send the terse
> version to the server log. I don't mind adjusting things for
> errhidecontext(), but it's not "just a bug".
>

Hard to say if it is bug or not - actually it is not consistent - the name
signalize so context will not be used - and there are no any other
possibility to specify if it should be only for client side or for all.

I don't would to do more complex than it is - just when is some exception
marked as "hide context" I expect, so context will not be shown everywhere.
Probably we should not to introduce function

errreallyhiddencontext() :)

Regards

Pavel

>
> Greetings,
>
> Andres Freund
>
>
> --
> 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 Mon, Jun 8, 2015 at 8:19 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:

> On 2015-06-08 14:44:53 +0000, Jeevan Chalke wrote:
> > The following review has been posted through the commitfest application:
> > make installcheck-world: tested, passed
> > Implements feature: tested, passed
> > Spec compliant: tested, passed
> > Documentation: tested, passed
> >
> > This is trivial bug fix in the area of hiding error context.
> >
> > I observed that there are two places from which we are calling this
> function
> > to hide the context in log messages. Those were broken.
>
> Broken in which sense? They did prevent stuff to go from the server log?
>
> I'm not convinced that hiding stuff from the client is really
> necessarily the same as hiding it from the server log. We e.g. always
> send the verbose log to the client, even if we only send the terse
> version to the server log. I don't mind adjusting things for
> errhidecontext(), but it's not "just a bug".
>
>
Function name itself says that we need to hide the context.
And this I assume it means from all the logs/client etc.

I said it is broken as these two calls are calling this function
with passing TRUE explicitly. But even though I can see the
context messages on the client.

Anyway, I don't want to argue on whether it is a bug or not.

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

--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2015-06-08 14:44:53 +0000, Jeevan Chalke wrote:
>> This is trivial bug fix in the area of hiding error context.
>>
>> I observed that there are two places from which we are calling this function
>> to hide the context in log messages. Those were broken.

> Broken in which sense? They did prevent stuff to go from the server log?

> I'm not convinced that hiding stuff from the client is really
> necessarily the same as hiding it from the server log. We e.g. always
> send the verbose log to the client, even if we only send the terse
> version to the server log. I don't mind adjusting things for
> errhidecontext(), but it's not "just a bug".

Not only is it not "just a bug", I disagree that it's a bug at all.
The documentation of the errhidestmt function is crystal clear about
what it does:

* errhidecontext --- optionally suppress CONTEXT: field of log entry

That says "log entry", not anything else. Furthermore, this is clearly
modeled on errhidestmt(), which also only affects what's written to the
log.

Generally our position on error reporting is that it's the client's
responsibility to decide what parts of a report it will or won't show
to the user, so even if we agreed the overall behavior was undesirable,
I do not think this is the appropriate fix.

I especially object to the part of the patch that suppresses calling the
context callback stack functions; that's just introducing inconsistent
behavior for no reason. It doesn't prevent collection of context (there
are lots of errcontext() calls directly in ereports, which this wouldn't
stop), and it will break callers that are using those callbacks for
anything more than just calling errcontext(). An example here is that in
clauses.c's sql_inline_error_callback, this would not only suppress the
CONTEXT line but also reporting of the error cursor location.

What is the actual use-case that prompted this complaint?

regards, tom lane

2015-07-03 1:07 GMT+02:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2015-06-08 14:44:53 +0000, Jeevan Chalke wrote:
> >> This is trivial bug fix in the area of hiding error context.
> >>
> >> I observed that there are two places from which we are calling this
> function
> >> to hide the context in log messages. Those were broken.
>
> > Broken in which sense? They did prevent stuff to go from the server log?
>
> > I'm not convinced that hiding stuff from the client is really
> > necessarily the same as hiding it from the server log. We e.g. always
> > send the verbose log to the client, even if we only send the terse
> > version to the server log. I don't mind adjusting things for
> > errhidecontext(), but it's not "just a bug".
>
> Not only is it not "just a bug", I disagree that it's a bug at all.
> The documentation of the errhidestmt function is crystal clear about
> what it does:
>
> * errhidecontext --- optionally suppress CONTEXT: field of log entry
>
> That says "log entry", not anything else. Furthermore, this is clearly
> modeled on errhidestmt(), which also only affects what's written to the
> log.
>
> Generally our position on error reporting is that it's the client's
> responsibility to decide what parts of a report it will or won't show
> to the user, so even if we agreed the overall behavior was undesirable,
> I do not think this is the appropriate fix.
>
> I especially object to the part of the patch that suppresses calling the
> context callback stack functions; that's just introducing inconsistent
> behavior for no reason. It doesn't prevent collection of context (there
> are lots of errcontext() calls directly in ereports, which this wouldn't
> stop), and it will break callers that are using those callbacks for
> anything more than just calling errcontext(). An example here is that in
> clauses.c's sql_inline_error_callback, this would not only suppress the
> CONTEXT line but also reporting of the error cursor location.
>

I didn't know it - My idea was, when CONTEXT is not showed, then is useless
to collect data for it.

>
> What is the actual use-case that prompted this complaint?
>

I would to use it for controlling (enabling, disabling) CONTEXT in RAISE
statement in plpgsql. I am thinking so one option for this purpose is
enough, and I would not to add other option to specify LOG, CLIENT.

Regards

Pavel

>
> regards, tom lane
>

On 2015-07-03 06:20:14 +0200, Pavel Stehule wrote:
> I would to use it for controlling (enabling, disabling) CONTEXT in RAISE
> statement in plpgsql. I am thinking so one option for this purpose is
> enough, and I would not to add other option to specify LOG, CLIENT.

I don't think a plpgsql function should be able to suppress all
context. From a security/debuggability POV that's a bad idea. The
context messages are the only way right now to have any chance of
tracing back what caused an error in a function because log_statements et
al. will not show it.

2015-07-07 14:13 GMT+02:00 Andres Freund <andres(at)anarazel(dot)de>:

> On 2015-07-03 06:20:14 +0200, Pavel Stehule wrote:
> > I would to use it for controlling (enabling, disabling) CONTEXT in RAISE
> > statement in plpgsql. I am thinking so one option for this purpose is
> > enough, and I would not to add other option to specify LOG, CLIENT.
>
> I don't think a plpgsql function should be able to suppress all
> context. From a security/debuggability POV that's a bad idea. The
> context messages are the only way right now to have any chance of
> tracing back what caused an error in a function because log_statements et
> al. will not show it.
>

It does it now. The context is not raised for exception raised by RAISE
statement from PL/pgSQL - and I would to fix it. But sometimes the context
is useless - for NOTICE level for example. I seen a strange workarounds -
RAISE NOTIFY followed by PERFORM 10/0 to get a context from PLpgSQL call.

Pavel