Re: [HACKERS] bugfix: incomplete implementation of errhidecontext
2015-07-07 14:13 GMT+02:00 Andres Freund : > 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
Re: [HACKERS] bugfix: incomplete implementation of errhidecontext
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bugfix: incomplete implementation of errhidecontext
2015-07-03 1:07 GMT+02:00 Tom Lane : > Andres Freund writes: > > On 2015-06-08 14:44:53 +, 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 >
Re: [HACKERS] bugfix: incomplete implementation of errhidecontext
Andres Freund writes: > On 2015-06-08 14:44:53 +, 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bugfix: incomplete implementation of errhidecontext
On Mon, Jun 8, 2015 at 8:19 PM, Andres Freund wrote: > On 2015-06-08 14:44:53 +, 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
Re: [HACKERS] bugfix: incomplete implementation of errhidecontext
2015-06-08 16:49 GMT+02:00 Andres Freund : > On 2015-06-08 14:44:53 +, 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 >
Re: [HACKERS] bugfix: incomplete implementation of errhidecontext
On 2015-06-08 14:44:53 +, 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bugfix: incomplete implementation of errhidecontext
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bugfix: incomplete implementation of errhidecontext
Done https://commitfest.postgresql.org/5/257/ 2015-05-29 9:56 GMT+02:00 Pavel Stehule : > > > 2015-05-29 9:53 GMT+02:00 Jeevan Chalke : > >> 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 >> > >
Re: [HACKERS] bugfix: incomplete implementation of errhidecontext
2015-05-29 9:53 GMT+02:00 Jeevan Chalke : > 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 >
Re: [HACKERS] bugfix: incomplete implementation of errhidecontext
Pavel, will it be good if you separately submit the "bugfix: incomplete implementation of errhidecontext" patch in this commitfest? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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 commit 7ee40ad6e5233f0ca2a5c10d1afcfb5d035164e6 Author: root Date: Thu Apr 30 11:59:45 2015 +0200 fix bug in errhidecontext() implementation diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index dfd102a..2a9d0fd 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -426,10 +426,11 @@ errfinish(int dummy,...) * functions will be treated as recursive errors --- this ensures we will * avoid infinite recursion (see errstart). */ - for (econtext = error_context_stack; - econtext != NULL; - econtext = econtext->previous) - (*econtext->callback) (econtext->arg); + if (!edata->hide_ctx) + for (econtext = error_context_stack; + econtext != NULL; + econtext = econtext->previous) + (*econtext->callback) (econtext->arg); /* * If ERROR (not more nor less) we pass it off to the current handler. @@ -3137,7 +3138,7 @@ send_message_to_frontend(ErrorData *edata) err_sendstring(&msgbuf, edata->hint); } - if (edata->context) + if (edata->context && !edata->hide_ctx) { pq_sendbyte(&msgbuf, PG_DIAG_CONTEXT); err_sendstring(&msgbuf, edata->context); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers