Bug #928: server_min_messages (log_min_messages in CVS) have PGC_USERSET GucContext

pgsql-bugs@postgresql.org writes:

In src/backend/utils/misc/guc.c "server_min_messages"
("log_min_messages" in CVS)configuration option have PGC_USERSET
GucContext. I think that it is not good idea that user can
change server log details level. I suggest change GucContext in
this case on PGC_SIGHUP.

PGC_SUSET would be appropriate if we think that there's really a security
issue here. But ISTM this was already considered when the present setup
was designed, and we deliberately chose USERSET. Bruce, do you remember
what the reasoning was?

regards, tom lane

Tom Lane wrote:

pgsql-bugs@postgresql.org writes:

In src/backend/utils/misc/guc.c "server_min_messages"
("log_min_messages" in CVS)configuration option have PGC_USERSET
GucContext. I think that it is not good idea that user can
change server log details level. I suggest change GucContext in
this case on PGC_SIGHUP.

PGC_SUSET would be appropriate if we think that there's really a security
issue here. But ISTM this was already considered when the present setup
was designed, and we deliberately chose USERSET. Bruce, do you remember
what the reasoning was?

The issue was that you might want to increase server logging in certain
clients to help debug a problem. If we had a "don't raise me" setting,
that would work.

Another idea is to add the ability to SET things perminantly.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

On Sun, 2003-03-30 at 19:20, Bruce Momjian wrote:

The issue was that you might want to increase server logging in certain
clients to help debug a problem.

That seems a little obscure to me -- IMHO it's not really worth adding
additional GUC complexity to account for it. Why not just use SUSET, and
then consider how to change it if someone complains?

Cheers,

Neil

pgsql-bugs@postgresql.org wrote:

Sergey N. Yatskevich (syatskevich@n21lab.gosniias.msk.ru) reports a bug with a severity of 4
The lower the number the more severe it is.

Short Description
server_min_messages (log_min_messages in CVS) have PGC_USERSET GucContext

Long Description
In src/backend/utils/misc/guc.c "server_min_messages"
("log_min_messages" in CVS)configuration option have PGC_USERSET
GucContext. I think that it is not good idea that user can
change server log details level. I suggest change GucContext in
this case on PGC_SIGHUP.

The reason it is PGC_USERSET is because we imagined people might want to
increase the amount of information sent to the server logs, and we don't
have an _increase_only_ restriction capability. However, maybe it
should be PGC_SUSET.

Comments?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

The reason it is PGC_USERSET is because we imagined people might want to
increase the amount of information sent to the server logs, and we don't
have an _increase_only_ restriction capability. However, maybe it
should be PGC_SUSET.

Yeah, probably so. Particularly with the 7.4 error message additions,
it'd be possible to make the logs very verbose indeed, which might be
seen as a form of attack (or at least a good way to hide your traces).

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

The reason it is PGC_USERSET is because we imagined people might want to
increase the amount of information sent to the server logs, and we don't
have an _increase_only_ restriction capability. However, maybe it
should be PGC_SUSET.

Yeah, probably so. Particularly with the 7.4 error message additions,
it'd be possible to make the logs very verbose indeed, which might be
seen as a form of attack (or at least a good way to hide your traces).

I reviewed all the user-settable GUC variables and generated the
following patch --- there were quite a few other setting that should be
super-user only.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I reviewed all the user-settable GUC variables and generated the
following patch --- there were quite a few other setting that should be
super-user only.

Where in the world did you get the idea that debug settings should be
SUSET? The log_xxx settings probably should be, but I don't agree with
the rest of these changes ...

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I reviewed all the user-settable GUC variables and generated the
following patch --- there were quite a few other setting that should be
super-user only.

Where in the world did you get the idea that debug settings should be
SUSET? The log_xxx settings probably should be, but I don't agree with

Do you think that is an appropriate way to respond to a patch?

the rest of these changes ...

The reason I changed the debug_ ones is that those go directly to the
server log file, not to the client. If you are worried about filling up
the server log files, those debug outputs could really fill things up
quickly. If something is going only to the server logs, does it make
sense for non-super users to be able to change it?

However, I now remember that you can set client_min_messages to DEBUG5
and see those debug messages. If we want to still allow debug_* display
to the client by non-super users, we have to give up the idea of
preventing server log filling. Of course, even with debug_ prevented,
it is still possible to fill up the log file, so probably restricting
the debug_* isn't worth it.

New patch attached, that does just the log_ ones.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

The reason I changed the debug_ ones is that those go directly to the
server log file, not to the client.

IIRC, all that stuff goes through elog (if there's anything left in the
standard build that writes straight to stderr, we need to be fixing the
code, not messing with GUC defaults). So AFAIK the log_min_messages
setting ought to control it all. What are you seeing that I don't?

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

The reason I changed the debug_ ones is that those go directly to the
server log file, not to the client.

IIRC, all that stuff goes through elog (if there's anything left in the
standard build that writes straight to stderr, we need to be fixing the
code, not messing with GUC defaults). So AFAIK the log_min_messages
setting ought to control it all. What are you seeing that I don't?

I tried this as a non-super user and saw the parser info in the logs.

Looking at the C code, the debug_* stuff goes to the logs as LOG.

I suppose you could set log_min_messages above LOG, but that seems too
dangerous. I don't have a problem changing only the log_* settings, but
we shouldn't assume there is any way to protect the log file. I think
we are restricting the log_* settings only so they can not be supressed
ffrom the logs.

---------------------------------------------------------------------------

test=> select current_user;
current_user
--------------
wilson
(1 row)

test=> set debug_print_parse to true;
SET
test=> select current_user;
current_user
--------------
wilson
(1 row)

test=> \q
(2) cat /u/pg/server.log
LOG: database system was shut down at 2003-05-27 00:23:02 EDT
IN: StartupXLOG (xlog.c:2510)
LOG: checkpoint record is at 0/263D924
IN: StartupXLOG (xlog.c:2538)
LOG: redo record is at 0/263D924; undo record is at 0/0; shutdown TRUE
IN: StartupXLOG (xlog.c:2558)
LOG: next transaction id: 6216; next oid: 154373
IN: StartupXLOG (xlog.c:2562)
LOG: database system is ready
IN: StartupXLOG (xlog.c:2819)
ERROR: CREATE USER: user name "wilson" already exists
IN: CreateUser (user.c:608)
ERROR: parser: parse error at or near "current_user"
IN: yyerror (scan.l:590)
LOG: parse tree:
{QUERY :commandType 1 :querySource 0 :canSetTag true :utilityStmt <>
:resultRelation 0 :into <> :hasAggs false :hasSubLinks false :rtable <>
:jointree {FROMEXPR :fromlist <> :quals <>} :rowMarks () :targetList
({TARGETENTRY :resdom {RESDOM :resno 1 :restype 19 :restypmod -1 :resname
current_user :ressortgroupref 0 :resorigtbl 0 :resorigcol 0 :resjunk false}
:expr {FUNCEXPR :funcid 745 :funcresulttype 19 :funcretset false :funcformat 0
:args <>}}) :groupClause <> :havingQual <> :distinctClause <> :sortClause <>
:limitOffset <> :limitCount <> :setOperations <> :resultRelations ()}

IN:  elog_node_display (print.c:85)
(
-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

IIRC, all that stuff goes through elog (if there's anything left in the
standard build that writes straight to stderr, we need to be fixing the
code, not messing with GUC defaults). So AFAIK the log_min_messages
setting ought to control it all. What are you seeing that I don't?

I tried this as a non-super user and saw the parser info in the logs.

Yeah, but if log_min_messages becomes SUSET, then a non-superuser can't
force debug info into the log. At least not if we change its elevel to
DEBUG rather than LOG, which I think would be a reasonable compromise.
The real problem here is the choice of LOG as the elevel for output
controlled by the debug_xxx switches.

I suppose you could set log_min_messages above LOG, but that seems too
dangerous. I don't have a problem changing only the log_* settings, but
we shouldn't assume there is any way to protect the log file. I think
we are restricting the log_* settings only so they can not be supressed
ffrom the logs.

The thing is that I thought it was a major step forward for users to be
able to get debug info at their consoles, rather than needing access to
the system log to see it. If we make these settings SUSET then we're
taking that away again, and that's really not good.

A fuller description of what I'm thinking of:

* log_min_messages should indeed be SUSET, and also the other log_xxx
switches.

* Output controlled by a log_xxx switch should have error level LOG.

* Output controlled by a debug_xxx switch should have error level DEBUG.
These switches remain USERSET.

* Adjust all other debug output (non-switch-controlled) to have error
level DEBUG2 or below; this way, setting log_min_messages = DEBUG gets
you only the specifically requested output.

* Note in the Admin Guide that log_min_messages <= DEBUG is needed
if you want any debug_xxx output to appear in the log, and that this
setting can produce very voluminuous output.

If this seems reasonable, I can adjust the elevels while I'm making the
real-soon-now editing pass through the elog calls.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

IIRC, all that stuff goes through elog (if there's anything left in the
standard build that writes straight to stderr, we need to be fixing the
code, not messing with GUC defaults). So AFAIK the log_min_messages
setting ought to control it all. What are you seeing that I don't?

I tried this as a non-super user and saw the parser info in the logs.

Yeah, but if log_min_messages becomes SUSET, then a non-superuser can't
force debug info into the log. At least not if we change its elevel to
DEBUG rather than LOG, which I think would be a reasonable compromise.
The real problem here is the choice of LOG as the elevel for output
controlled by the debug_xxx switches.

Yes. I set debug_* output to LOG at the time I did the error leveling
because it traditionally has gone to the server logs. Now that we can
send to the client, it makes sense to move it to DEBUG1. The downside
is that setting the debug_ variables will send output _nowhere_ until
you change client_min_messages or log_min_messages. It clearly makes
the debug_* variables treat the client and serve logs the same way in
terms of output preference, and I think this makes sense. We just need
to be prepared to communicate that change in the release notes.

I suppose you could set log_min_messages above LOG, but that seems too
dangerous. I don't have a problem changing only the log_* settings, but
we shouldn't assume there is any way to protect the log file. I think
we are restricting the log_* settings only so they can not be supressed
ffrom the logs.

The thing is that I thought it was a major step forward for users to be
able to get debug info at their consoles, rather than needing access to
the system log to see it. If we make these settings SUSET then we're
taking that away again, and that's really not good.

Right.

A fuller description of what I'm thinking of:

* log_min_messages should indeed be SUSET, and also the other log_xxx
switches.

OK, will apply.

* Output controlled by a log_xxx switch should have error level LOG.

I thought it already did. If not, it certainly should.

* Output controlled by a debug_xxx switch should have error level DEBUG.
These switches remain USERSET.

OK. One issue is that I think you should make them all DEBUG1, and move
any existing DEBUG1 references to DEBUG2, perhaps moving DEBUG2 up as
well. Here are the totals for the various DEBUG* uses:

$ gid DEBUG1|wc -l
82
$ gid DEBUG2|wc -l
27
$ gid DEBUG3|wc -l
63
$ gid DEBUG4|wc -l
4
$ gid DEBUG5|wc -l
5

Seems we should push them all up, and merge DEBUG 4 and 5, and use
DEBUG1 for the debug_* output. That seems logical.

* Adjust all other debug output (non-switch-controlled) to have error
level DEBUG2 or below; this way, setting log_min_messages = DEBUG gets
you only the specifically requested output.

Oh, I see you already had that idea! :-)

* Note in the Admin Guide that log_min_messages <= DEBUG is needed
if you want any debug_xxx output to appear in the log, and that this
setting can produce very voluminuous output.

Good idea. This allows us to easily get debug_* stuff to the client
_without_ getting it in the server.

If this seems reasonable, I can adjust the elevels while I'm making the
real-soon-now editing pass through the elog calls.

I can do the change easily here. It will take just a few minutes. Let
me know and I can complete it in an hour.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

* Output controlled by a log_xxx switch should have error level LOG.

I thought it already did. If not, it certainly should.

I think it probably already does everywhere; I'm just trying to lay out
the policy in full.

OK. One issue is that I think you should make them all DEBUG1, and move
any existing DEBUG1 references to DEBUG2, perhaps moving DEBUG2 up as
well. Here are the totals for the various DEBUG* uses:

Something like that. I was planning to take a second look at the debug
level assignments and make sure they were all rational (ie, some
relationship between level and usefulness...)

I can do the change easily here. It will take just a few minutes. Let
me know and I can complete it in an hour.

If you like; doesn't matter to me who does it. I'm busy with a
different project today.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

* Output controlled by a log_xxx switch should have error level LOG.

I thought it already did. If not, it certainly should.

I think it probably already does everywhere; I'm just trying to lay out
the policy in full.

OK.

OK. One issue is that I think you should make them all DEBUG1, and move
any existing DEBUG1 references to DEBUG2, perhaps moving DEBUG2 up as
well. Here are the totals for the various DEBUG* uses:

Something like that. I was planning to take a second look at the debug
level assignments and make sure they were all rational (ie, some
relationship between level and usefulness...)

OK, let me renumber them and you can still review them. I did find that
DEBUG4 was used only as a place holder, so I just shifted DEBUG1-3 to
DEBUG 2-4, and made the three debug_ variables to DEBUG1 (debug_pretty_print
doesn't count). I also did trace_notify.

I can do the change easily here. It will take just a few minutes. Let
me know and I can complete it in an hour.

If you like; doesn't matter to me who does it. I'm busy with a
different project today.

Patch attached and applied, with doc changes. This deals with filling
the server logs and makes debug_ more agnostic about its output
location.

I am focused on this today, so I might as well do it. :-)

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

A fuller description of what I'm thinking of:

* log_min_messages should indeed be SUSET, and also the other log_xxx
switches.

Let me address this. By making log SUSET, we prevent non-super users
from turning it off, but we also prevent them from turning it on,
especially using per-user/db methods.

The only solution I can think of would be to have a new permission level
between USERSET and SUSET, where the boolean could be turned on by
non-super users, but not turned off, but then again, that adds the
ability to fill up the server logs.

I am not sure how much that is used, so I am not inclined to add it at
this time, but some day, it might be needed.

Patch to tighten log_* attached and applied.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073