Thread: bugfix: incomplete implementation of errhidecontext
Hi
current implementation of errhidecontext is not complete:Attachment
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.chalke@gmail.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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
2015-05-29 9:56 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:
2015-05-29 9:53 GMT+02:00 Jeevan Chalke <jeevan.chalke@gmail.com>:Pavel, will it be good if you separately submit the
"bugfix: incomplete implementation of errhidecontext"
patch in this commitfest?ok, I'll do itPavel
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.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@anarazel.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@postgresql.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@anarazel.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@postgresql.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
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
Andres Freund <andres@anarazel.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@sss.pgh.pa.us>:
Andres Freund <andres@anarazel.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@anarazel.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