Re: [HACKERS] [PROPOSAL] Client Log Output Filtering

[David Steele]

On 4/4/16 12:36 PM, Tom Lane wrote:

David Steele  writes:

On 4/4/16 11:21 AM, Tom Lane wrote:

I had in mind a patch that simply added LOG_SERVER_ONLY as another define
and did whatever seemed appropriate documentation-wise.  I see no reason
to touch the places that are currently dealing with client communication
failures.



I still prefer to collapse them into a single value for the current
implementation.


Right, that's what I had in mind, sorry if I was unclear.  I agree that
considering LOG_SERVER_ONLY as the main name and COMMERROR as an alias
is reasonable.


COMMERROR was not documented in sources.sgml so LOG_SERVER_ONLY wasn't
either.  I'm happy to do that that though it's not clear to me where it
would go.  I could just put it in the general description.


Ah, I thought I remembered that the specific elevels were documented
there, but I see they're only documented in elog.h.  Doesn't seem like
it's incumbent on this patch to improve that.

Committed with a quick pgindent fixup.


Thank you, and I appreciate you suggesting this simple solution.

The advantage of this over the other possible solutions is that I can 
easily port the feature back to 9.5 by defining LOG_SERVER_ONLY when 
building pgaudit for that version.  That's a big win in my mind.


--
-David
da...@pgmasters.net


--
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] [PROPOSAL] Client Log Output Filtering

[Tom Lane]

David Steele  writes:
> On 4/4/16 11:21 AM, Tom Lane wrote:
>> I had in mind a patch that simply added LOG_SERVER_ONLY as another define
>> and did whatever seemed appropriate documentation-wise.  I see no reason
>> to touch the places that are currently dealing with client communication
>> failures.

> I still prefer to collapse them into a single value for the current 
> implementation.

Right, that's what I had in mind, sorry if I was unclear.  I agree that
considering LOG_SERVER_ONLY as the main name and COMMERROR as an alias
is reasonable.

> COMMERROR was not documented in sources.sgml so LOG_SERVER_ONLY wasn't 
> either.  I'm happy to do that that though it's not clear to me where it 
> would go.  I could just put it in the general description.

Ah, I thought I remembered that the specific elevels were documented
there, but I see they're only documented in elog.h.  Doesn't seem like
it's incumbent on this patch to improve that.

Committed with a quick pgindent fixup.

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] [PROPOSAL] Client Log Output Filtering

[David Steele]

On 4/4/16 11:21 AM, Tom Lane wrote:

David Steele  writes:

On 3/29/16 12:58 PM, Tom Lane wrote:

...  Basically,
my point is that LOG_ONLY achieves 95% of the benefit for probably
0.01% of the work.



Attached is a patch that re-purposes COMMERROR as LOG_SERVER_ONLY.  I
went ahead and replaced all instances of COMMERROR with LOG_SERVER_ONLY.


Uh, what?  COMMERROR is a distinct concept in my opinion.  It might happen
to share the same implementation today, but that doesn't make it the
same thing.


Fair enough.


I had in mind a patch that simply added LOG_SERVER_ONLY as another define
and did whatever seemed appropriate documentation-wise.  I see no reason
to touch the places that are currently dealing with client communication
failures.


I still prefer to collapse them into a single value for the current 
implementation.  Otherwise there are several places that need to check 
for both in elog.c and their behavior is identical (for now).  For my 2c 
it makes more sense to collapse COMMERROR into LOG_SERVER_ONLY since 
that more accurately describes what it actually does in the elog code.


What do you think of the attached?

COMMERROR was not documented in sources.sgml so LOG_SERVER_ONLY wasn't 
either.  I'm happy to do that that though it's not clear to me where it 
would go.  I could just put it in the general description.


Thanks,
--
-David
da...@pgmasters.net
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 8e00609..740f089 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -293,7 +293,7 @@ errstart(int elevel, const char *filename, int lineno,
output_to_server = is_log_level_output(elevel, log_min_messages);
 
/* Determine whether message is enabled for client output */
-   if (whereToSendOutput == DestRemote && elevel != COMMERROR)
+   if (whereToSendOutput == DestRemote && elevel != LOG_SERVER_ONLY)
{
/*
 * client_min_messages is honored only after we complete the
@@ -2086,7 +2086,7 @@ write_eventlog(int level, const char *line, int len)
case DEBUG2:
case DEBUG1:
case LOG:
-   case COMMERROR:
+   case LOG_SERVER_ONLY:
case INFO:
case NOTICE:
eventlevel = EVENTLOG_INFORMATION_TYPE;
@@ -2965,7 +2965,7 @@ send_message_to_server_log(ErrorData *edata)
syslog_level = LOG_DEBUG;
break;
case LOG:
-   case COMMERROR:
+   case LOG_SERVER_ONLY:
case INFO:
syslog_level = LOG_INFO;
break;
@@ -3595,7 +3595,7 @@ error_severity(int elevel)
prefix = _("DEBUG");
break;
case LOG:
-   case COMMERROR:
+   case LOG_SERVER_ONLY:
prefix = _("LOG");
break;
case INFO:
@@ -3699,7 +3699,7 @@ write_stderr(const char *fmt,...)
 static bool
 is_log_level_output(int elevel, int log_min_level)
 {
-   if (elevel == LOG || elevel == COMMERROR)
+   if (elevel == LOG || elevel == LOG_SERVER_ONLY)
{
if (log_min_level == LOG || log_min_level <= ERROR)
return true;
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 901651f..1d7fcca 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -25,9 +25,11 @@
 #define DEBUG1 14  /* used by GUC debug_* 
variables */
 #define LOG15  /* Server operational 
messages; sent only to
 * server log 
by default. */
-#define COMMERROR  16  /* Client communication 
problems; same as LOG
-* for server 
reporting, but never sent to
-* client. */
+#define LOG_SERVER_ONLY16  /* Same as LOG for server 
reporting, but never
+  sent to 
client. */
+#define COMMERROR  LOG_SERVER_ONLY /* Client communication problems; same 
as
+  LOG 
for server reporting, but never sent
+  to 
client. */
 #define INFO   17  /* Messages specifically 
requested by user (eg
 * VACUUM 
VERBOSE output); always sent to
 * client 
regardless of 

Re: [HACKERS] [PROPOSAL] Client Log Output Filtering

[Tom Lane]

David Steele  writes:
> On 3/29/16 12:58 PM, Tom Lane wrote:
>> ...  Basically,
>> my point is that LOG_ONLY achieves 95% of the benefit for probably
>> 0.01% of the work.

> Attached is a patch that re-purposes COMMERROR as LOG_SERVER_ONLY.  I 
> went ahead and replaced all instances of COMMERROR with LOG_SERVER_ONLY.

Uh, what?  COMMERROR is a distinct concept in my opinion.  It might happen
to share the same implementation today, but that doesn't make it the
same thing.

I had in mind a patch that simply added LOG_SERVER_ONLY as another define
and did whatever seemed appropriate documentation-wise.  I see no reason
to touch the places that are currently dealing with client communication
failures.

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] [PROPOSAL] Client Log Output Filtering

[David Steele]

Hi Tom,

On 3/29/16 12:58 PM, Tom Lane wrote:


Oh, I agree that there's a compelling use-case for LOG |
ERR_HIDE_FROM_CLIENT.  I'm less certain that there's a use-case
for supporting such a flag across all elevels that is strong enough
to justify all the work we'd have to do to make it happen.  Basically,
my point is that LOG_ONLY achieves 95% of the benefit for probably
0.01% of the work.


Attached is a patch that re-purposes COMMERROR as LOG_SERVER_ONLY.  I 
went ahead and replaced all instances of COMMERROR with LOG_SERVER_ONLY.


I left the COMMERROR #define but it is no longer used by any server, 
client, or included contrib code (I also noted that it was DEPRECATED in 
the comment).  I'll leave it up to the committer to remove if deemed 
appropriate.


I realize there's no agreement on how this should work ideally, but this 
patch answers the current need without introducing anything new and 
shouldn't cause regressions.  It does address confusion that would arise 
from using COMMERROR in ereports that clearly are not.


Thanks,
--
-David
da...@pgmasters.net
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 2751183..db6da9f 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -635,7 +635,7 @@ recv_password_packet(Port *port)
 * log.
 */
if (mtype != EOF)
-   ereport(COMMERROR,
+   ereport(LOG_SERVER_ONLY,

(errcode(ERRCODE_PROTOCOL_VIOLATION),
errmsg("expected password response, got 
message type %d",
   mtype)));
@@ -663,7 +663,7 @@ recv_password_packet(Port *port)
 * StringInfo is guaranteed to have an appended '\0'.
 */
if (strlen(buf.data) + 1 != buf.len)
-   ereport(COMMERROR,
+   ereport(LOG_SERVER_ONLY,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
 errmsg("invalid password packet size")));
 
@@ -853,7 +853,7 @@ pg_GSS_recvauth(Port *port)
{
/* Only log error if client didn't disconnect. */
if (mtype != EOF)
-   ereport(COMMERROR,
+   ereport(LOG_SERVER_ONLY,

(errcode(ERRCODE_PROTOCOL_VIOLATION),
 errmsg("expected GSS response, 
got message type %d",
mtype)));
@@ -1092,7 +1092,7 @@ pg_SSPI_recvauth(Port *port)
{
/* Only log error if client didn't disconnect. */
if (mtype != EOF)
-   ereport(COMMERROR,
+   ereport(LOG_SERVER_ONLY,

(errcode(ERRCODE_PROTOCOL_VIOLATION),
 errmsg("expected SSPI 
response, got message type %d",
mtype)));
diff --git a/src/backend/libpq/be-secure-openssl.c 
b/src/backend/libpq/be-secure-openssl.c
index 6009663..b8f7a07 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -359,7 +359,7 @@ be_tls_open_server(Port *port)
 
if (!(port->ssl = SSL_new(SSL_context)))
{
-   ereport(COMMERROR,
+   ereport(LOG_SERVER_ONLY,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
 errmsg("could not initialize SSL connection: 
%s",
SSLerrmessage(;
@@ -367,7 +367,7 @@ be_tls_open_server(Port *port)
}
if (!my_SSL_set_fd(port, port->sock))
{
-   ereport(COMMERROR,
+   ereport(LOG_SERVER_ONLY,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
 errmsg("could not set SSL socket: %s",
SSLerrmessage(;
@@ -401,27 +401,27 @@ aloop:
goto aloop;
case SSL_ERROR_SYSCALL:
if (r < 0)
-   ereport(COMMERROR,
+   ereport(LOG_SERVER_ONLY,

(errcode_for_socket_access(),
 errmsg("could not 
accept SSL connection: %m")));
else
-   ereport(COMMERROR,
+   ereport(LOG_SERVER_ONLY,

Re: [HACKERS] [PROPOSAL] Client Log Output Filtering

[Tom Lane]

Andres Freund  writes:
> There's a number of cases during early startup/auth where we really
> don't want client to get messages.

Right, which we handle at present with ClientAuthInProgress.  But
I think it's worth drawing a distinction between "don't send message
to client because of the state we're in" and "don't send this message
to client because it's security-sensitive".  The latter is better
handled by annotations attached to specific ereport sites, the former
not.

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] [PROPOSAL] Client Log Output Filtering

[Andres Freund]

On 2016-03-29 12:58:22 -0400, Tom Lane wrote:
> Looking back at the earlier thread Andres mentioned, I see that he was
> specifically on about being able to do ereport(ERROR | LOG_NO_CLIENT),
> which I've got a problem with because of the point about not breaking
> wire-protocol expectations.

Yes.  The reason for doing it in that case is that we weren't allowed to
send errors to the client in that specific case - by the protocol state.

There's a number of cases during early startup/auth where we really
don't want client to get messages.


> and it doesn't look like he thought about the elevel-comparisons
> issue either.

Nope, I didn't. That's a long while ago ;)

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] [PROPOSAL] Client Log Output Filtering

[Robert Haas]

On Tue, Mar 29, 2016 at 12:58 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Tue, Mar 29, 2016 at 12:38 PM, Tom Lane  wrote:
>>> If we invent LOG_ONLY (feel free to bikeshed the name), we could later
>>> redefine it as (LOG | ERR_HIDE_FROM_CLIENT), if we ever upgrade the
>>> underlying implementation to allow that.  But I remain concerned about
>>> dealing with logic like "if (elevel < ERROR)", and I am unconvinced that
>>> there's a near-term use-case here that's compelling enough to justify
>>> finding all the places that do that.
>
>> Yeah, I think it's dead certain that such code exists, and, ahem, not
>> only in our tree.  I suspect that EDB is not the only organization
>> that has written code that involves comparing error levels.
>
> Yeah, that's exactly my concern: it'd likely break third-party code
> not only our own.
>
>> Putting
>> the flags in the low-order bits seems like it might be workable, but I
>> think using a high-order bit is right out.
>
> We'd need a bit more investigation.  I'm not exactly sure that we can
> renumber the existing elevel codes at all without breaking things
> (guc.c's management of client_min_messages et al would be a place
> to look at, for instance).  But if someone wants to pursue it,
> the general concept seems somewhat sane.
>
> Looking back at the earlier thread Andres mentioned, I see that he was
> specifically on about being able to do ereport(ERROR | LOG_NO_CLIENT),
> which I've got a problem with because of the point about not breaking
> wire-protocol expectations.  You could maybe define such a case as
> substituting a text like "error message is hidden" when sending the
> error to the client?  But the draft patch he had didn't address that
> point, and it doesn't look like he thought about the elevel-comparisons
> issue either.
>
>> I don't agree that there is no compelling use case for log-only
>> output.  I think there's a lot of use case for that, either for
>> general auditing (like pgaudit) or for any specific case where you
>> want to log sensitive information that shouldn't ever be allowed to
>> leak to the client.
>
> Oh, I agree that there's a compelling use-case for LOG |
> ERR_HIDE_FROM_CLIENT.  I'm less certain that there's a use-case
> for supporting such a flag across all elevels that is strong enough
> to justify all the work we'd have to do to make it happen.  Basically,
> my point is that LOG_ONLY achieves 95% of the benefit for probably
> 0.01% of the work.

If LOG_ONLY is what we can get right now, I'm happy to take the money and run.

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


-- 
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] [PROPOSAL] Client Log Output Filtering

[Tom Lane]

Robert Haas  writes:
> On Tue, Mar 29, 2016 at 12:38 PM, Tom Lane  wrote:
>> If we invent LOG_ONLY (feel free to bikeshed the name), we could later
>> redefine it as (LOG | ERR_HIDE_FROM_CLIENT), if we ever upgrade the
>> underlying implementation to allow that.  But I remain concerned about
>> dealing with logic like "if (elevel < ERROR)", and I am unconvinced that
>> there's a near-term use-case here that's compelling enough to justify
>> finding all the places that do that.

> Yeah, I think it's dead certain that such code exists, and, ahem, not
> only in our tree.  I suspect that EDB is not the only organization
> that has written code that involves comparing error levels.

Yeah, that's exactly my concern: it'd likely break third-party code
not only our own.

> Putting
> the flags in the low-order bits seems like it might be workable, but I
> think using a high-order bit is right out.

We'd need a bit more investigation.  I'm not exactly sure that we can
renumber the existing elevel codes at all without breaking things
(guc.c's management of client_min_messages et al would be a place
to look at, for instance).  But if someone wants to pursue it,
the general concept seems somewhat sane.

Looking back at the earlier thread Andres mentioned, I see that he was
specifically on about being able to do ereport(ERROR | LOG_NO_CLIENT),
which I've got a problem with because of the point about not breaking
wire-protocol expectations.  You could maybe define such a case as
substituting a text like "error message is hidden" when sending the
error to the client?  But the draft patch he had didn't address that
point, and it doesn't look like he thought about the elevel-comparisons
issue either.

> I don't agree that there is no compelling use case for log-only
> output.  I think there's a lot of use case for that, either for
> general auditing (like pgaudit) or for any specific case where you
> want to log sensitive information that shouldn't ever be allowed to
> leak to the client.

Oh, I agree that there's a compelling use-case for LOG |
ERR_HIDE_FROM_CLIENT.  I'm less certain that there's a use-case
for supporting such a flag across all elevels that is strong enough
to justify all the work we'd have to do to make it happen.  Basically,
my point is that LOG_ONLY achieves 95% of the benefit for probably
0.01% of the work.

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] [PROPOSAL] Client Log Output Filtering

[Alvaro Herrera]

Robert Haas wrote:

> Yeah, I think it's dead certain that such code exists, and, ahem, not
> only in our tree.  I suspect that EDB is not the only organization
> that has written code that involves comparing error levels.  Putting
> the flags in the low-order bits seems like it might be workable, but I
> think using a high-order bit is right out.

We could redefine elevel as a struct when assertions are enabled, and
provide functions to do the < comparisons in that case; they would
reduce to regular integers when assertions are disabled.  That would
raise compile-time errors in all places that need to be updated.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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] [PROPOSAL] Client Log Output Filtering

[Robert Haas]

On Tue, Mar 29, 2016 at 12:38 PM, Tom Lane  wrote:
> Andres Freund  writes:
>> On 2016-03-29 12:28:40 -0400, Tom Lane wrote:
>>> My proposal would be to invent a new elevel macro, maybe LOG_ONLY,
>>> for this purpose.  But under the hood it'd be the same as COMMERROR.
>
>> A couple years back I proposed making thinks like COMERROR into flags |
>> ed into elevel, rather than distinct levels.  I still think that's a
>> better approach; and it doesn't force us to forgo using distinct log
>> levels.
>
> If we invent LOG_ONLY (feel free to bikeshed the name), we could later
> redefine it as (LOG | ERR_HIDE_FROM_CLIENT), if we ever upgrade the
> underlying implementation to allow that.  But I remain concerned about
> dealing with logic like "if (elevel < ERROR)", and I am unconvinced that
> there's a near-term use-case here that's compelling enough to justify
> finding all the places that do that.

Yeah, I think it's dead certain that such code exists, and, ahem, not
only in our tree.  I suspect that EDB is not the only organization
that has written code that involves comparing error levels.  Putting
the flags in the low-order bits seems like it might be workable, but I
think using a high-order bit is right out.

I don't agree that there is no compelling use case for log-only
output.  I think there's a lot of use case for that, either for
general auditing (like pgaudit) or for any specific case where you
want to log sensitive information that shouldn't ever be allowed to
leak to the client.

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


-- 
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] [PROPOSAL] Client Log Output Filtering

[Andres Freund]

On 2016-03-29 12:38:48 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2016-03-29 12:28:40 -0400, Tom Lane wrote:
> If we invent LOG_ONLY (feel free to bikeshed the name), we could later
> redefine it as (LOG | ERR_HIDE_FROM_CLIENT), if we ever upgrade the
> underlying implementation to allow that.  But I remain concerned about
> dealing with logic like "if (elevel < ERROR)", and I am unconvinced that
> there's a near-term use-case here that's compelling enough to justify
> finding all the places that do that.

Hm. Putting the distinct levels in the upper bits, and the flags in the
lower bits should deal with that concern?   We already have a bunch of
pretty ugly bits about errors that shouldn't go to clients, I'd rather
have something that allows addressing those as well.


-- 
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] [PROPOSAL] Client Log Output Filtering

[Tom Lane]

Andres Freund  writes:
> On 2016-03-29 12:28:40 -0400, Tom Lane wrote:
>> My proposal would be to invent a new elevel macro, maybe LOG_ONLY,
>> for this purpose.  But under the hood it'd be the same as COMMERROR.

> A couple years back I proposed making thinks like COMERROR into flags |
> ed into elevel, rather than distinct levels.  I still think that's a
> better approach; and it doesn't force us to forgo using distinct log
> levels.

If we invent LOG_ONLY (feel free to bikeshed the name), we could later
redefine it as (LOG | ERR_HIDE_FROM_CLIENT), if we ever upgrade the
underlying implementation to allow that.  But I remain concerned about
dealing with logic like "if (elevel < ERROR)", and I am unconvinced that
there's a near-term use-case here that's compelling enough to justify
finding all the places that do that.

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] [PROPOSAL] Client Log Output Filtering

[Andres Freund]

On 2016-03-29 18:33:19 +0200, Andres Freund wrote:
> A couple years back I proposed making thinks like COMERROR into flags |
> ed into elevel, rather than distinct levels.  I still think that's a
> better approach; and it doesn't force us to forgo using distinct log
> levels.

http://archives.postgresql.org/message-id/201002132237.43930.andres%40anarazel.de


-- 
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] [PROPOSAL] Client Log Output Filtering

[Andres Freund]

On 2016-03-29 12:28:40 -0400, Tom Lane wrote:
> Alvaro Herrera  writes:
> > David Steele wrote:
> >> On 3/29/16 10:18 AM, Tom Lane wrote:
> >>> Repurposing COMMERROR is definitely starting to seem like a low-impact
> >>> solution compared to these others.  Under what circumstances would you
> >>> be wanting hide-from-client with an elevel different from LOG, anyway?
> 
> > So audit records would use COMMERROR?  That sounds really bad to me.
> 
> My proposal would be to invent a new elevel macro, maybe LOG_ONLY,
> for this purpose.  But under the hood it'd be the same as COMMERROR.

A couple years back I proposed making thinks like COMERROR into flags |
ed into elevel, rather than distinct levels.  I still think that's a
better approach; and it doesn't force us to forgo using distinct log
levels.

Andres


-- 
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] [PROPOSAL] Client Log Output Filtering

[David Steele]

On 3/29/16 12:28 PM, Tom Lane wrote:

Alvaro Herrera  writes:

David Steele wrote:

On 3/29/16 10:18 AM, Tom Lane wrote:

Repurposing COMMERROR is definitely starting to seem like a low-impact
solution compared to these others.  Under what circumstances would you
be wanting hide-from-client with an elevel different from LOG, anyway?



So audit records would use COMMERROR?  That sounds really bad to me.


My proposal would be to invent a new elevel macro, maybe LOG_ONLY,
for this purpose.  But under the hood it'd be the same as COMMERROR.


My mistake, I see what you are getting at now.

--
-David
da...@pgmasters.net


--
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] [PROPOSAL] Client Log Output Filtering

[Tom Lane]

Alvaro Herrera  writes:
> David Steele wrote:
>> On 3/29/16 10:18 AM, Tom Lane wrote:
>>> Repurposing COMMERROR is definitely starting to seem like a low-impact
>>> solution compared to these others.  Under what circumstances would you
>>> be wanting hide-from-client with an elevel different from LOG, anyway?

> So audit records would use COMMERROR?  That sounds really bad to me.

My proposal would be to invent a new elevel macro, maybe LOG_ONLY,
for this purpose.  But under the hood it'd be the same as COMMERROR.

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] [PROPOSAL] Client Log Output Filtering

[David Steele]

On 3/29/16 11:37 AM, Alvaro Herrera wrote:

David Steele wrote:

On 3/29/16 10:18 AM, Tom Lane wrote:



Repurposing COMMERROR is definitely starting to seem like a low-impact
solution compared to these others.  Under what circumstances would you
be wanting hide-from-client with an elevel different from LOG, anyway?


In pgaudit the log level for audit messages is user configurable but this
was mostly added for testing purposes on the client side.  I don't think it
would be a big deal to force the level to LOG when client output is
suppressed.


So audit records would use COMMERROR?  That sounds really bad to me.


I'm not a big fan of it myself but my ideas don't seem to be getting any 
traction...


--
-David
da...@pgmasters.net


--
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] [PROPOSAL] Client Log Output Filtering

[Alvaro Herrera]

David Steele wrote:
> On 3/29/16 10:18 AM, Tom Lane wrote:

> >Repurposing COMMERROR is definitely starting to seem like a low-impact
> >solution compared to these others.  Under what circumstances would you
> >be wanting hide-from-client with an elevel different from LOG, anyway?
> 
> In pgaudit the log level for audit messages is user configurable but this
> was mostly added for testing purposes on the client side.  I don't think it
> would be a big deal to force the level to LOG when client output is
> suppressed.

So audit records would use COMMERROR?  That sounds really bad to me.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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] [PROPOSAL] Client Log Output Filtering

[David Steele]

On 3/29/16 10:18 AM, Tom Lane wrote:

David Steele  writes:

On 3/28/16 2:00 PM, Tom Lane wrote:

One idea is to invent a new elevel which is never sent to the client ---
analogously to COMMERROR, though probably much higher priority than that,
maybe the same priority as LOG.  If there actually is a use for a range of
elevels on errhidefromclient'd messages, that wouldn't work very well of
course.  Or we could consider having a flag bit that is OR'd into the
elevel <...>



I think a flag would be more flexible than introducing a new log level.


I thought about this some more, and while the flag-bit approach definitely
has some attraction, it also has a big problem: there are lots of places
with code like "if (elevel >= ERROR)" which would be at risk of getting
confused, and I'm not confident we'd find them all.  We could possibly
dodge that by shifting the elevel constants up a couple of bits and
putting the flag in the LSB rather than a high-order bit; but that's a
bit icky/risky too.

Repurposing COMMERROR is definitely starting to seem like a low-impact
solution compared to these others.  Under what circumstances would you
be wanting hide-from-client with an elevel different from LOG, anyway?


In pgaudit the log level for audit messages is user configurable but 
this was mostly added for testing purposes on the client side.  I don't 
think it would be a big deal to force the level to LOG when client 
output is suppressed.


The advantage of this approach is that it will also work on older 
versions of PG so I don't have to wait to introduce the feature.  I'll 
try it out and see how it goes.


Thanks,
--
-David
da...@pgmasters.net


--
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] [PROPOSAL] Client Log Output Filtering

[Tom Lane]

David Steele  writes:
> On 3/28/16 2:00 PM, Tom Lane wrote:
>> One idea is to invent a new elevel which is never sent to the client ---
>> analogously to COMMERROR, though probably much higher priority than that,
>> maybe the same priority as LOG.  If there actually is a use for a range of
>> elevels on errhidefromclient'd messages, that wouldn't work very well of
>> course.  Or we could consider having a flag bit that is OR'd into the
>> elevel <...>

> I think a flag would be more flexible than introducing a new log level.

I thought about this some more, and while the flag-bit approach definitely
has some attraction, it also has a big problem: there are lots of places
with code like "if (elevel >= ERROR)" which would be at risk of getting
confused, and I'm not confident we'd find them all.  We could possibly
dodge that by shifting the elevel constants up a couple of bits and
putting the flag in the LSB rather than a high-order bit; but that's a
bit icky/risky too.

Repurposing COMMERROR is definitely starting to seem like a low-impact
solution compared to these others.  Under what circumstances would you
be wanting hide-from-client with an elevel different from LOG, anyway?

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] [PROPOSAL] Client Log Output Filtering

[David Steele]

On 3/28/16 2:00 PM, Tom Lane wrote:


I set to work on committing this, but was soon rather dissatisfied with
it, because as-implemented there is no way to short-circuit elog.c's
processing if the message is not to be sent to either the client or the
postmaster log.  Ideally this would be taken into account when errstart()
figures the output_to_client setting to begin with --- but of course we
can't do that with this API, because errhidefromclient() hasn't run yet.


That's a weakness of this approach but I'm not sure it's a big deal 
since there will generally still be output on the server.  If both are 
suppressed I think that would be a sign of misconfiguration.



The patch is buggy even without that consideration, because it would
potentially disable client output of messages even after they've been
promoted to FATAL by pg_re_throw.  We could fix that by clearing the flag
in pg_re_throw, but I think that's just another symptom of the shutoff
being done in the wrong place.


Or elevel could be checked in EmitErrorReport():

if (edata->output_to_client &&
!(edata->hide_from_client && edata->elevel < ERROR))
send_message_to_frontend(edata);


I wonder just what the expected usage scenario is for this function, and
whether it would not be better addressed by inventing some other API
rather than modeling it on errhidestmt(), which is by no means the same
kind of thing.


The particular use case I have in mind is to suppress client output in 
pgaudit.  The original patch took a different approach by trying to 
merge with the logic to suppress messages in auth.  Maybe you should 
take a look at that patch and see what you think:


http://www.postgresql.org/message-id/5655b621.3080...@pgmasters.net


One idea is to invent a new elevel which is never sent to the client ---
analogously to COMMERROR, though probably much higher priority than that,
maybe the same priority as LOG.  If there actually is a use for a range of
elevels on errhidefromclient'd messages, that wouldn't work very well of
course.  Or we could consider having a flag bit that is OR'd into the
elevel <...>


I think a flag would be more flexible than introducing a new log level.

--
-David
da...@pgmasters.net


--
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] [PROPOSAL] Client Log Output Filtering

[Tom Lane]

David Steele  writes:
> On 3/10/16 11:00 AM, Petr Jelinek wrote:
>> The comment above errhidefromclient says "Only log levels below ERROR
>> can be hidden from the client." but use of the errhidefromclient(true)
>> actually does hide the error message from client, client just gets
>> failed query without any message when used with ERROR level.

> Right you are.  The v3 patch adds this check.

> I also added the documentation to sources.sgml that Tom pointed out was
> missing.

I set to work on committing this, but was soon rather dissatisfied with
it, because as-implemented there is no way to short-circuit elog.c's
processing if the message is not to be sent to either the client or the
postmaster log.  Ideally this would be taken into account when errstart()
figures the output_to_client setting to begin with --- but of course we
can't do that with this API, because errhidefromclient() hasn't run yet.

The patch is buggy even without that consideration, because it would
potentially disable client output of messages even after they've been
promoted to FATAL by pg_re_throw.  We could fix that by clearing the flag
in pg_re_throw, but I think that's just another symptom of the shutoff
being done in the wrong place.

I wonder just what the expected usage scenario is for this function, and
whether it would not be better addressed by inventing some other API
rather than modeling it on errhidestmt(), which is by no means the same
kind of thing.

One idea is to invent a new elevel which is never sent to the client ---
analogously to COMMERROR, though probably much higher priority than that,
maybe the same priority as LOG.  If there actually is a use for a range of
elevels on errhidefromclient'd messages, that wouldn't work very well of
course.  Or we could consider having a flag bit that is OR'd into the
elevel, along the lines of

ereport(LOG | ERR_HIDE_FROM_CLIENT, (errmsg()));

so that the information is available at errstart time.

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] [PROPOSAL] Client Log Output Filtering

[David Steele]

On 3/10/16 9:51 AM, Tom Lane wrote:

> The patch is evidently modeled on errhidestmt and errhidectx, which are
> making the same assumption for their fields.
> 
> I wonder whether, instead of continuing to proliferate random bool fields
> in struct ErrorData, we oughta replace them all with an "int flags" field.
> That's probably material for a separate patch though.

There are currently six boolean flags (if you include my new one) that
all relate to visibility in one way or another.  Combining them into a
"flags" field makes sense to me.

I'll submit a proposal to hackers after 9.6 to make this change.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] [PROPOSAL] Client Log Output Filtering

[David Steele]

On 3/10/16 11:07 AM, Tom Lane wrote:
> Petr Jelinek  writes:
>> The comment above errhidefromclient says "Only log levels below ERROR 
>> can be hidden from the client." but use of the errhidefromclient(true) 
>> actually does hide the error message from client, client just gets 
>> failed query without any message when used with ERROR level.
> 
> Um.  That seems pretty broken --- I think it's a violation of the wire
> protocol spec.
> 
> I notice though that we allow client_min_messages to be set to FATAL,
> which would be a different way of violating the protocol.  Maybe we
> should reduce the max setting of that to ERROR?

This was the same conclusion I came to for the log_level setting in pgaudit.

I'll submit a proposal to hackers after 9.6 to make this change.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] [PROPOSAL] Client Log Output Filtering

[David Steele]

On 3/10/16 11:00 AM, Petr Jelinek wrote:

> The comment above errhidefromclient says "Only log levels below ERROR
> can be hidden from the client." but use of the errhidefromclient(true)
> actually does hide the error message from client, client just gets
> failed query without any message when used with ERROR level.

Right you are.  The v3 patch adds this check.

I also added the documentation to sources.sgml that Tom pointed out was
missing.

Thanks,
-- 
-David
da...@pgmasters.net
diff --git a/doc/src/sgml/sources.sgml b/doc/src/sgml/sources.sgml
index fcb3e40..4ad15d0 100644
--- a/doc/src/sgml/sources.sgml
+++ b/doc/src/sgml/sources.sgml
@@ -353,6 +353,14 @@ ereport(ERROR,
  includes the current statement already.
 

+   
+
+ errhidefromclient(bool hide_from_client) can be
+ called to suppress message output to the client when the error severity
+ level is lower than ERROR.  It is useful for hiding sensitive
+ messages from the client while still logging them on the server.
+
+   
   

 
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5b7554b..5e5035d 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1094,6 +1094,22 @@ errhidecontext(bool hide_ctx)
return 0;   /* return value does 
not matter */
 }
 
+/*
+ * errhidefromclient --- optionally suppress output of message to client
+ * if error severity level < ERROR.
+ */
+int
+errhidefromclient(bool hide_from_client)
+{
+   ErrorData  *edata = [errordata_stack_depth];
+
+   /* we don't bother incrementing recursion_depth */
+   CHECK_STACK_DEPTH();
+
+   edata->hide_from_client = hide_from_client && edata->elevel < ERROR;
+
+   return 0;   /* return value does 
not matter */
+}
 
 /*
  * errfunction --- add reporting function name to the current error
@@ -1477,7 +1493,7 @@ EmitErrorReport(void)
send_message_to_server_log(edata);
 
/* Send to client, if enabled */
-   if (edata->output_to_client)
+   if (edata->output_to_client && !edata->hide_from_client)
send_message_to_frontend(edata);
 
MemoryContextSwitchTo(oldcontext);
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 7d338dd..05dfe2b 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -179,6 +179,7 @@ extern int  errcontext_msg(const char *fmt,...) 
pg_attribute_printf(1, 2);
 
 extern int errhidestmt(bool hide_stmt);
 extern int errhidecontext(bool hide_ctx);
+extern int errhidefromclient(bool hide_from_client);
 
 extern int errfunction(const char *funcname);
 extern int errposition(int cursorpos);
@@ -343,6 +344,7 @@ typedef struct ErrorData
boolshow_funcname;  /* true to force funcname inclusion */
boolhide_stmt;  /* true to prevent STATEMENT: 
inclusion */
boolhide_ctx;   /* true to prevent CONTEXT: 
inclusion */
+   boolhide_from_client;   /* true to prevent client 
output */
const char *filename;   /* __FILE__ of ereport() call */
int lineno; /* __LINE__ of 
ereport() call */
const char *funcname;   /* __func__ of ereport() call */


signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] [PROPOSAL] Client Log Output Filtering

[Petr Jelinek]

On 10/03/16 17:07, Tom Lane wrote:

Petr Jelinek  writes:

The comment above errhidefromclient says "Only log levels below ERROR
can be hidden from the client." but use of the errhidefromclient(true)
actually does hide the error message from client, client just gets
failed query without any message when used with ERROR level.


Um.  That seems pretty broken --- I think it's a violation of the wire
protocol spec.



I was thinking that as well. The doc says that on error the 
ErrorResponse is sent and connection is closed and we clearly fail to 
send the ErrorResponse in this case.



I notice though that we allow client_min_messages to be set to FATAL,
which would be a different way of violating the protocol.  Maybe we
should reduce the max setting of that to ERROR?



Hmm yeah that seems to be that way for very long time though so I wonder 
if that's intentional although it's also against protocol spec then. In 
any case I would be in favor of lowering the max setting to ERROR.


For the patch at hand, it should be sufficient for errhidefromclient() 
to check that the edata->elevel is lower than ERROR.


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
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] [PROPOSAL] Client Log Output Filtering

[Tom Lane]

Petr Jelinek  writes:
> The comment above errhidefromclient says "Only log levels below ERROR 
> can be hidden from the client." but use of the errhidefromclient(true) 
> actually does hide the error message from client, client just gets 
> failed query without any message when used with ERROR level.

Um.  That seems pretty broken --- I think it's a violation of the wire
protocol spec.

I notice though that we allow client_min_messages to be set to FATAL,
which would be a different way of violating the protocol.  Maybe we
should reduce the max setting of that to ERROR?

libpq/psql seem to more or less survive this situation:

regression=# set client_min_messages to fatal;
SET
regression=# select 2/0;
regression=# select 1;
 ?column? 
--
1
(1 row)

but it doesn't really seem like a great idea.

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] [PROPOSAL] Client Log Output Filtering

[Petr Jelinek]

On 10/03/16 15:08, David Steele wrote:

Looking at the code, this adds bool hide_from_client to edata which is
not initialized in errstart so that needs to be fixed.


I figured this would take care of it:

MemSet(edata, 0, sizeof(ErrorData));

Since I want hide_from_client to default to false.  Am I missing something?



Right, missed that, sorry for the noise.

I have another issue though.

The comment above errhidefromclient says "Only log levels below ERROR 
can be hidden from the client." but use of the errhidefromclient(true) 
actually does hide the error message from client, client just gets 
failed query without any message when used with ERROR level.


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
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] [PROPOSAL] Client Log Output Filtering

[Tom Lane]

David Steele  writes:
> I have attached a patch that adds an ereport() macro to suppress client
> output for a single report call (applies cleanly on 1d0c3b3).  I'll also
> move it to the next CF.

This patch fails to add the necessary documentation (see sources.sgml)

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] [PROPOSAL] Client Log Output Filtering

[Tom Lane]

David Steele  writes:
> On 3/9/16 7:37 PM, Petr Jelinek wrote:
>> Looking at the code, this adds bool hide_from_client to edata which is
>> not initialized in errstart so that needs to be fixed.

> I figured this would take care of it:
> MemSet(edata, 0, sizeof(ErrorData));
> Since I want hide_from_client to default to false.  Am I missing something?

The patch is evidently modeled on errhidestmt and errhidectx, which are
making the same assumption for their fields.

I wonder whether, instead of continuing to proliferate random bool fields
in struct ErrorData, we oughta replace them all with an "int flags" field.
That's probably material for a separate patch though.

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] [PROPOSAL] Client Log Output Filtering

[David Steele]

On 3/9/16 7:37 PM, Petr Jelinek wrote:

On 03/02/16 05:02, Robert Haas wrote:

On Mon, Feb 1, 2016 at 7:24 PM, David Steele  wrote:


I have attached a patch that adds an ereport() macro to suppress client
output for a single report call (applies cleanly on 1d0c3b3).  I'll also
move it to the next CF.


I don't see any reason not to accept this.



Yes, the idea seems sane.

Looking at the code, this adds bool hide_from_client to edata which is
not initialized in errstart so that needs to be fixed.


I figured this would take care of it:

MemSet(edata, 0, sizeof(ErrorData));

Since I want hide_from_client to default to false.  Am I missing something?

--
-David
da...@pgmasters.net


--
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] [PROPOSAL] Client Log Output Filtering

[Petr Jelinek]

On 03/02/16 05:02, Robert Haas wrote:

On Mon, Feb 1, 2016 at 7:24 PM, David Steele  wrote:


I have attached a patch that adds an ereport() macro to suppress client
output for a single report call (applies cleanly on 1d0c3b3).  I'll also
move it to the next CF.


I don't see any reason not to accept this.



Yes, the idea seems sane.

Looking at the code, this adds bool hide_from_client to edata which is 
not initialized in errstart so that needs to be fixed.


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
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] [PROPOSAL] Client Log Output Filtering

[Robert Haas]

On Mon, Feb 1, 2016 at 7:24 PM, David Steele  wrote:
> On 2/1/16 5:25 PM, Alvaro Herrera wrote:
>> David Steele wrote:
>
>>> 2) There would be two different ways to suppress client messages but I was
>>> hoping to only have one.
>>
>> I think they are two different things actually.
>
> Fair enough - that was my initial reaction as well but then I thought
> the other way would be better.
>
>> I'm closing this as returned with feedback.
>
> I have attached a patch that adds an ereport() macro to suppress client
> output for a single report call (applies cleanly on 1d0c3b3).  I'll also
> move it to the next CF.

I don't see any reason not to accept this.

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


-- 
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] [PROPOSAL] Client Log Output Filtering

[Alvaro Herrera]

David Steele wrote:
> On 1/11/16 6:50 PM, Alvaro Herrera wrote:
> >David Steele wrote:
> >>The patch creates a new counter to separate the log filtering from the
> >>authentication functionality.  This makes it possible to get the same
> >>filtering in other parts of the code (or extensions) without abusing the
> >>ClientAuthInProgress variable or using the log hook.
> >
> >Hmm, okay, but this would need a large blinking comment explaining that
> >the calling code have better set a PG_TRY block when incrementing, so
> >that on errors it resets to zero again.  Or maybe some handling in
> >AbortOutOfAnyTransaction/AbortCurrentTransaction.  or both.
> 
> In the case of pgaudit only the ereport call is wrapped in the
> LimitClientLogOutput() calls which I thought was safe - am I wrong about
> that?

Yeah, errstart itself could fail.  It's not common but it does happen.

> 1) Unless pgaudit is committed there wouldn't be any code calling the
> errhidefromclient() function and that seemed like a bad plan.

Well, you could add a test module to ensure it continues to work.

> 2) There would be two different ways to suppress client messages but I was
> hoping to only have one.

I think they are two different things actually.

I'm closing this as returned with feedback.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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] [PROPOSAL] Client Log Output Filtering

[David Steele]

On 2/1/16 5:25 PM, Alvaro Herrera wrote:
> David Steele wrote:

>> 2) There would be two different ways to suppress client messages but I was
>> hoping to only have one.
> 
> I think they are two different things actually.

Fair enough - that was my initial reaction as well but then I thought
the other way would be better.

> I'm closing this as returned with feedback.

I have attached a patch that adds an ereport() macro to suppress client
output for a single report call (applies cleanly on 1d0c3b3).  I'll also
move it to the next CF.

Thanks!
-- 
-David
da...@pgmasters.net
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 9005b26..c53ef95 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1091,6 +1091,23 @@ errhidecontext(bool hide_ctx)
return 0;   /* return value does 
not matter */
 }
 
+/*
+ * errhidefromclient --- optionally suppress output of message to client
+ *
+ * Only log levels below ERROR can be hidden from the client.
+ */
+int
+errhidefromclient(bool hide_from_client)
+{
+   ErrorData  *edata = [errordata_stack_depth];
+
+   /* we don't bother incrementing recursion_depth */
+   CHECK_STACK_DEPTH();
+
+   edata->hide_from_client = hide_from_client;
+
+   return 0;   /* return value does 
not matter */
+}
 
 /*
  * errfunction --- add reporting function name to the current error
@@ -1467,7 +1484,7 @@ EmitErrorReport(void)
send_message_to_server_log(edata);
 
/* Send to client, if enabled */
-   if (edata->output_to_client)
+   if (edata->output_to_client && !edata->hide_from_client)
send_message_to_frontend(edata);
 
MemoryContextSwitchTo(oldcontext);
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 326896f..14b87b7 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -179,6 +179,7 @@ extern int  errcontext_msg(const char *fmt,...) 
pg_attribute_printf(1, 2);
 
 extern int errhidestmt(bool hide_stmt);
 extern int errhidecontext(bool hide_ctx);
+extern int errhidefromclient(bool hide_from_client);
 
 extern int errfunction(const char *funcname);
 extern int errposition(int cursorpos);
@@ -343,6 +344,7 @@ typedef struct ErrorData
boolshow_funcname;  /* true to force funcname inclusion */
boolhide_stmt;  /* true to prevent STATEMENT: 
inclusion */
boolhide_ctx;   /* true to prevent CONTEXT: 
inclusion */
+   boolhide_from_client;   /* true to prevent client 
output */
const char *filename;   /* __FILE__ of ereport() call */
int lineno; /* __LINE__ of 
ereport() call */
const char *funcname;   /* __func__ of ereport() call */


signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] [PROPOSAL] Client Log Output Filtering

[David Steele]

On 1/11/16 6:50 PM, Alvaro Herrera wrote:

David Steele wrote:

The patch creates a new counter to separate the log filtering from the
authentication functionality.  This makes it possible to get the same
filtering in other parts of the code (or extensions) without abusing the
ClientAuthInProgress variable or using the log hook.


Hmm, okay, but this would need a large blinking comment explaining that
the calling code have better set a PG_TRY block when incrementing, so
that on errors it resets to zero again.  Or maybe some handling in
AbortOutOfAnyTransaction/AbortCurrentTransaction.  or both.


In the case of pgaudit only the ereport call is wrapped in the 
LimitClientLogOutput() calls which I thought was safe - am I wrong about 
that?



I also considered a new function for ereport (like errhidecontext()) but
this mechanism would not have worked for authentication and so would not
have been used anywhere in core.



If the audit code
calls something that throws an error (other than an audit message -- say
"out of memory"), then it would also be hidden, but you may not want it
to be hidden.


This shouldn't happen -- see above.

I think maybe it's better to have each individual error

message be marked as "don't show to client" rather than a global var.


That's easy enough to do and I already have the code for it since that's 
the first thing I tried.  However, there were two reasons I didn't 
submit that version:


1) Unless pgaudit is committed there wouldn't be any code calling the 
errhidefromclient() function and that seemed like a bad plan.


2) There would be two different ways to suppress client messages but I 
was hoping to only have one.


As you say, authentication is a different beast so maybe #2 is not a big 
deal.  I could combine the alternative ereport patch with the pgaudit 
patch to address #1 but I would like to have the capability in core 
whether pgaudit is committed or not, which is why I submitted it separately.


Any advice would be greatly appreciated.

Thanks,
--
-David
da...@pgmasters.net


--
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] [PROPOSAL] Client Log Output Filtering

[Robert Haas]

On Mon, Jan 11, 2016 at 6:50 PM, Alvaro Herrera
 wrote:
> I think maybe it's better to have each individual error
> message be marked as "don't show to client" rather than a global var.

+1.

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


-- 
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] [PROPOSAL] Client Log Output Filtering

[Alvaro Herrera]

David Steele wrote:
> Currently log messages generated by pgaudit can be made visible to the
> client simply by altering client_min_messages.  While this has not been a
> showstopper for anyone it's ideal, either.
> 
> The client authentication code sets the global variable ClientAuthInProgress
> which causes ereport() to filter client output < ERROR while forcing client
> output >= ERROR.  This functionality would also work well for pgaudit.
> 
> The patch creates a new counter to separate the log filtering from the
> authentication functionality.  This makes it possible to get the same
> filtering in other parts of the code (or extensions) without abusing the
> ClientAuthInProgress variable or using the log hook.

Hmm, okay, but this would need a large blinking comment explaining that
the calling code have better set a PG_TRY block when incrementing, so
that on errors it resets to zero again.  Or maybe some handling in
AbortOutOfAnyTransaction/AbortCurrentTransaction.  or both.

> I also considered a new function for ereport (like errhidecontext()) but
> this mechanism would not have worked for authentication and so would not
> have been used anywhere in core.

But then, you already know that authentication phase is not exactly the
same use case as security auditing, so they don't have to work the same
way necessarily.  I think this needs more discussion.  If the audit code
calls something that throws an error (other than an audit message -- say
"out of memory"), then it would also be hidden, but you may not want it
to be hidden.  I think maybe it's better to have each individual error
message be marked as "don't show to client" rather than a global var.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

[HACKERS] [PROPOSAL] Client Log Output Filtering

[David Steele]

Currently log messages generated by pgaudit can be made visible to the 
client simply by altering client_min_messages.  While this has not been 
a showstopper for anyone it's ideal, either.


The client authentication code sets the global variable 
ClientAuthInProgress which causes ereport() to filter client output < 
ERROR while forcing client output >= ERROR.  This functionality would 
also work well for pgaudit.


The patch creates a new counter to separate the log filtering from the 
authentication functionality.  This makes it possible to get the same 
filtering in other parts of the code (or extensions) without abusing the 
ClientAuthInProgress variable or using the log hook.


I also considered a new function for ereport (like errhidecontext()) but 
this mechanism would not have worked for authentication and so would not 
have been used anywhere in core.


If there are no objections I'll submit it to the next commitfest.

--
-David
da...@pgmasters.net
diff --git a/src/backend/postmaster/postmaster.c 
b/src/backend/postmaster/postmaster.c
index 3cba0e5..6a8399d 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4013,8 +4013,16 @@ BackendInitialize(Port *port)
if (PreAuthDelay > 0)
pg_usleep(PreAuthDelay * 100L);
 
-   /* This flag will remain set until InitPostgres finishes authentication 
*/
-   ClientAuthInProgress = true;/* limit visibility of log messages */
+   /*
+* ClientAuthInProgress is a flag which is set from this point until
+* InitPostgres finishes with authentication to let
+* StatementTimeoutHandler() and quickdie() know that we are performing
+* authentication and to act accordingly.
+*/
+   ClientAuthInProgress = true;
+
+   /* Hide non-ERROR messages from the client */
+   LimitClientLogOutput(true);
 
/* save process start time */
port->SessionStartTime = GetCurrentTimestamp();
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 8b5b6c5..db1ecbb 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -69,7 +69,6 @@
 #include "libpq/pqformat.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
-#include "postmaster/postmaster.h"
 #include "postmaster/syslogger.h"
 #include "storage/ipc.h"
 #include "storage/proc.h"
@@ -93,6 +92,13 @@ sigjmp_buf *PG_exception_stack = NULL;
 extern bool redirection_done;
 
 /*
+ * When LimitClientLogOutputCounter > 0, client output < ERROR will be
+ * suppressed and >= ERROR will be output without regard for
+ * client_min_messages. See LimitClientLogOutput() and errstart().
+ */
+int LimitClientLogOutputCounter = 0;
+
+/*
  * Hook for intercepting messages before they are sent to the server log.
  * Note that the hook will not get called for messages that are suppressed
  * by log_min_messages.  Also note that logging hooks implemented in preload
@@ -299,7 +305,7 @@ errstart(int elevel, const char *filename, int lineno,
 * reasons and because many clients can't handle NOTICE messages
 * during authentication.
 */
-   if (ClientAuthInProgress)
+   if (LimitClientLogOutputCounter > 0)
output_to_client = (elevel >= ERROR);
else
output_to_client = (elevel >= client_min_messages ||
@@ -1738,7 +1744,7 @@ pg_re_throw(void)
edata->output_to_server = (FATAL >= log_min_messages);
if (whereToSendOutput == DestRemote)
{
-   if (ClientAuthInProgress)
+   if (LimitClientLogOutputCounter > 0)
edata->output_to_client = true;
else
edata->output_to_client = (FATAL >= 
client_min_messages);
@@ -1836,6 +1842,27 @@ GetErrorContextStack(void)
 
 
 /*
+ * LimitClientLogOutput - Limit client log output to >= ERROR
+ *
+ * When LimitClientLogOutputCounter > 0, client output < ERROR will be
+ * suppressed and >= ERROR will be output without regard for
+ * client_min_messages. See LimitClientLogOutputCounter and errstart().
+ */
+void
+LimitClientLogOutput(bool state)
+{
+   if (state)
+   LimitClientLogOutputCounter++;
+   else
+   {
+   Assert(LimitClientLogOutputCounter > 0);
+
+   LimitClientLogOutputCounter--;
+   }
+}
+
+
+/*
  * Initialization of error output file
  */
 void
diff --git a/src/backend/utils/init/postinit.c 
b/src/backend/utils/init/postinit.c
index 7b19714..5471758 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -181,8 +181,8 @@ GetDatabaseTupleByOid(Oid dboid)
 static void
 PerformAuthentication(Port *port)
 {
-   /* This should be set already, but let's make sure */
-   ClientAuthInProgress = true;/* limit visibility of log