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
Attachments:
elog-errhidecontext-bugfix.patchtext/x-patch; charset=US-ASCII; name=elog-errhidecontext-bugfix.patchDownload
commit 7ee40ad6e5233f0ca2a5c10d1afcfb5d035164e6
Author: root <root@localhost.localdomain>
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);
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
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
Show quoted text
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.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.stehule@gmail.com>:
Show quoted text
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
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
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, passedThis 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
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, passedThis 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
Show quoted text
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, passedThis 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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Show quoted text
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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