Add support for logging the current role
Greetings,
Minor enhancement, but a valuable one imv. Hopefully there aren't any
issues with it. :)
Thanks!
Stephen
commit 3cb707aa9f228e629e7127625a76a223751a778b
Author: Stephen Frost <sfrost@snowman.net>
Date: Wed Jan 12 09:17:31 2011 -0500
Add support for logging the current role
This adds a '%o' option to the log_line_prefix GUC which will log the
current role. The '%u' option only logs the Session user, which can
be misleading, but it's valuable to have both options.
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***************
*** 3508,3513 **** local0.* /var/log/postgresql
--- 3508,3518 ----
<entry>yes</entry>
</row>
<row>
+ <entry><literal>%o</literal></entry>
+ <entry>Current role name</entry>
+ <entry>yes</entry>
+ </row>
+ <row>
<entry><literal>%d</literal></entry>
<entry>Database name</entry>
<entry>yes</entry>
*** a/src/backend/utils/error/elog.c
--- b/src/backend/utils/error/elog.c
***************
*** 1826,1831 **** log_line_prefix(StringInfo buf, ErrorData *edata)
--- 1826,1841 ----
appendStringInfoString(buf, username);
}
break;
+ case 'o':
+ if (MyProcPort)
+ {
+ const char *rolename = GetUserNameFromId(GetUserId());
+
+ if (rolename == NULL || *rolename == '\0')
+ rolename = _("[unknown]");
+ appendStringInfoString(buf, rolename);
+ }
+ break;
case 'd':
if (MyProcPort)
{
On Wed, Jan 12, 2011 at 9:23 AM, Stephen Frost <sfrost@snowman.net> wrote:
Minor enhancement, but a valuable one imv. Hopefully there aren't any
issues with it. :)
1. Why %o? That's not obviously mnemonic. Perhaps %U?
2. It won't be clear to people reading this what the difference is
between %u and this. You probably need to reword the documentation
for the existing option as well as documenting the new one.
3. Please attach the patch rather than including it inline, if possible.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
* Robert Haas (robertmhaas@gmail.com) wrote:
On Wed, Jan 12, 2011 at 9:23 AM, Stephen Frost <sfrost@snowman.net> wrote:
Minor enhancement, but a valuable one imv. Hopefully there aren't any
issues with it. :)1. Why %o? That's not obviously mnemonic. Perhaps %U?
r was taken? :) I'm not sure I like %U, but in the end I don't *really*
care. I'll update it to %U and wait for someone else to complain.
2. It won't be clear to people reading this what the difference is
between %u and this. You probably need to reword the documentation
for the existing option as well as documenting the new one.
Fair enough.
3. Please attach the patch rather than including it inline, if possible.
Hrm, I could have sworn that Tom had asked for the exact opposite in the
past, but either way is fine by me.
Stephen
On Wed, Jan 12, 2011 at 10:12 AM, Stephen Frost <sfrost@snowman.net> wrote:
r was taken? :) I'm not sure I like %U, but in the end I don't *really*
care. I'll update it to %U and wait for someone else to complain.
The joys of community...
3. Please attach the patch rather than including it inline, if possible.
Hrm, I could have sworn that Tom had asked for the exact opposite in the
past, but either way is fine by me.
Really? I don't remember that, but it's certainly possible. My
problem is that cutting and pasting from a browser window into a patch
file tends to be a little iffy. If you paste too much or too little
or the whitespace doesn't come out quite right, the patch doesn't
apply.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Greetings,
* Robert Haas (robertmhaas@gmail.com) wrote:
1. Why %o? That's not obviously mnemonic. Perhaps %U?
2. It won't be clear to people reading this what the difference is
between %u and this. You probably need to reword the documentation
for the existing option as well as documenting the new one.3. Please attach the patch rather than including it inline, if possible.
Updated patch attached-
commit 7319e8ddc91d62addea25b85f7dbe2f95132cdc1
Author: Stephen Frost <sfrost@snowman.net>
Date: Wed Jan 12 10:23:13 2011 -0500
Use %U for role in log_line_prefix; improve docs
Change the variable for logging the current role in log_line_prefix
from %o to %U, to better reflect the 'user'-type mnemonic.
Improve the documentation for the %U and %u log_line_prefix options
to better differentiate them from each other.
commit 3cb707aa9f228e629e7127625a76a223751a778b
Author: Stephen Frost <sfrost@snowman.net>
Date: Wed Jan 12 09:17:31 2011 -0500
Add support for logging the current role
This adds a '%o' option to the log_line_prefix GUC which will log the
current role. The '%u' option only logs the Session user, which can
be misleading, but it's valuable to have both options.
Thanks!
Stephen
Attachments:
log_role_option.patchtext/x-diff; charset=us-asciiDownload+17-7
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Jan 12, 2011 at 10:12 AM, Stephen Frost <sfrost@snowman.net> wrote:
Hrm, I could have sworn that Tom had asked for the exact opposite in the
past, but either way is fine by me.
Really? I don't remember that, but it's certainly possible.
I don't remember saying exactly that either. The main point is to
ensure the patch doesn't get mangled in transmission. I've seen people
screw it up both ways: inline is much more vulnerable to mailers
deciding to whack whitespace around, while attachments are vulnerable to
being encoded in all sorts of weird ways, some of which come out nicely
in the archives and some of which don't. I'm not in favor of gzipping
small patches that could perfectly well be left in readable form.
This particular patch looks fine here:
http://archives.postgresql.org/pgsql-hackers/2011-01/msg00845.php
so I'm thinking Stephen doesn't need to revisit his technique.
+1 for choosing something more mnemonic than "%o", btw.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
+1 for choosing something more mnemonic than "%o", btw.
Alright, not to be *too* ridiculous about this, but I'm feeling like
'%R' might be better than '%U', if we don't mind overloading a single
letter based on case. I've always been annoyed at the lack of
distinction between 'user' and 'role' in our docs and feel it does lead
to some confusion.
Updated patch attached, if people agree. Compiles, passes regressions,
works as advertised, etc.
commit bba27fe63702405514ed2c3bb72b70cc178f9ce1
Author: Stephen Frost <sfrost@snowman.net>
Date: Wed Jan 12 10:38:24 2011 -0500
Change log_line_prefix for current role to %R
As we're going for a mnemonic, and this is really about roles
instead of users, change log_line_prefix argument to %R from
%U for current_role.
Thanks,
Stephen
Attachments:
log_role_option.patchtext/x-diff; charset=us-asciiDownload+17-7
On Wed, Jan 12, 2011 at 10:43 AM, Stephen Frost <sfrost@snowman.net> wrote:
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
+1 for choosing something more mnemonic than "%o", btw.
Alright, not to be *too* ridiculous about this, but I'm feeling like
'%R' might be better than '%U', if we don't mind overloading a single
letter based on case. I've always been annoyed at the lack of
distinction between 'user' and 'role' in our docs and feel it does lead
to some confusion.Updated patch attached, if people agree. Compiles, passes regressions,
works as advertised, etc.
I was thinking that %u/%U would have the advantage of implying some
connection between the two things which is in fact present. %r/%R
seems not quite as good to me. Also, let's paint it tangerine.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
* Robert Haas (robertmhaas@gmail.com) wrote:
I was thinking that %u/%U would have the advantage of implying some
connection between the two things which is in fact present. %r/%R
seems not quite as good to me. Also, let's paint it tangerine.
I figured that's where you were going.
+1 for whatever the committer wants to commit. ;)
Stephen
On Wed, Jan 12, 2011 at 11:00 AM, Stephen Frost <sfrost@snowman.net> wrote:
* Robert Haas (robertmhaas@gmail.com) wrote:
I was thinking that %u/%U would have the advantage of implying some
connection between the two things which is in fact present. %r/%R
seems not quite as good to me. Also, let's paint it tangerine.I figured that's where you were going.
+1 for whatever the committer wants to commit. ;)
OK, done. :-)
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Jan 12, 2011 at 11:00 AM, Stephen Frost <sfrost@snowman.net> wrote:
+1 for whatever the committer wants to commit. ;)
OK, done. :-)
Uh, did you actually stop to *think* about this patch?
What you have just committed puts a syscache lookup into the elog output
path. Quite aside from the likely performance hit, this will
malfunction badly in any case where we're trying to log from an aborted
transaction.
Please revert and rethink.
regards, tom lane
On Wed, Jan 12, 2011 at 11:53 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Jan 12, 2011 at 11:00 AM, Stephen Frost <sfrost@snowman.net> wrote:
+1 for whatever the committer wants to commit. ;)
OK, done. :-)
Uh, did you actually stop to *think* about this patch?
You have a valid point here, but this isn't the most tactful way of putting it.
What you have just committed puts a syscache lookup into the elog output
path. Quite aside from the likely performance hit, this will
malfunction badly in any case where we're trying to log from an aborted
transaction.Please revert and rethink.
I think it's going to take more than a rethink - I don't see any way
to salvage it. :-(
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Uh, did you actually stop to *think* about this patch?
Actually, I was worried about exactly that, but I didn't see anything at
the top of elog.c that indicated if it'd be a problem or not (and the
Syscache lookup issue was *exactly* what I was looking for). :( There
was much discussion about recursion and memory commit and whatnot, but
nothing about SysCache lookups.
What you have just committed puts a syscache lookup into the elog output
path. Quite aside from the likely performance hit, this will
malfunction badly in any case where we're trying to log from an aborted
transaction.
I had been looking into storing the current role inside the Proc struct
or in some new variable and then pulling it from there (updating it when
needed during a SET ROLE, of course), but it seemed a bit of overkill if
it wasn't necessary (which wasn't obvious to me). We could also just log
the role's OID (%o anyone..?), since that doesn't need a syscache lookup
to get at. I'd much rather log the role name if we can tho.
I had looked through some of the other calls happening in log_line_prefix
and didn't see any explicit syscache lookups but it seemed like we were
doing quite a few other things that might have issues, so I had assumed
it'd be alright. Sorry about that.
Thanks,
Stephen
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
What you have just committed puts a syscache lookup into the elog output
path. Quite aside from the likely performance hit, this will
malfunction badly in any case where we're trying to log from an aborted
transaction.
Attached is my (admittedly horrible) attempt to add some comments to
elog.c regarding this issue. Reviewing this, I'm not sure the
performance concern is really an issue (given that the user could choose
to enable it or not), but clearly the other issue is a concern.
Thanks,
Stephen
commit 4dcf23e007967892557b7b113a9229cb9fc4575d
Author: Stephen Frost <sfrost@snowman.net>
Date: Wed Jan 12 12:22:16 2011 -0500
Improve comments at the top of elog.c
Add in some comments about how certain usually available backend
systems may be unavailable or which won't function properly in
elog.c due to the current transaction being in a failed state.
On Wed, Jan 12, 2011 at 12:13 PM, Stephen Frost <sfrost@snowman.net> wrote:
What you have just committed puts a syscache lookup into the elog output
path. Quite aside from the likely performance hit, this will
malfunction badly in any case where we're trying to log from an aborted
transaction.I had been looking into storing the current role inside the Proc struct
or in some new variable and then pulling it from there (updating it when
needed during a SET ROLE, of course), but it seemed a bit of overkill if
it wasn't necessary (which wasn't obvious to me). We could also just log
the role's OID (%o anyone..?), since that doesn't need a syscache lookup
to get at. I'd much rather log the role name if we can tho.
Logging the OID seems to be of questionable value. I thought of the
update-the-variable-when-it-changes approach too, but besides being a
bit expensive if it's changing frequently, it's not necessarily safe
to do the syscache lookup there either - see the comments for
GetUserIdAndSecContext (which are really for SetUserIdAndSecContext,
but they're in an odd place).
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Jan 12, 2011 at 12:25 PM, Stephen Frost <sfrost@snowman.net> wrote:
Attached is ...
I don't see an attachment, other than signature.asc.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
* Robert Haas (robertmhaas@gmail.com) wrote:
On Wed, Jan 12, 2011 at 12:25 PM, Stephen Frost <sfrost@snowman.net> wrote:
Attached is ...
I don't see an attachment, other than signature.asc.
I suck, sorry about that, here it is..
See, inlining is better! I wouldn't have forgotten it! ;)
Stephen
Attachments:
elog_comments.patchtext/x-diff; charset=us-asciiDownload+9-0
* Robert Haas (robertmhaas@gmail.com) wrote:
Logging the OID seems to be of questionable value.
I certainly disagree about this, not being able to figure out what's
causing a 'permissions denied' error because you don't know which role
the log is coming from is *very* annoying. Having to go look up the
role from the OID in the log is also annoying, but less so, imv. :)
I thought of the
update-the-variable-when-it-changes approach too, but besides being a
bit expensive if it's changing frequently, it's not necessarily safe
to do the syscache lookup there either - see the comments for
GetUserIdAndSecContext (which are really for SetUserIdAndSecContext,
but they're in an odd place).
Alright, here's a patch which adds the ability to log the current role's
OID and which calls GetUserIdAndSecContext() directly and handles the
possibility that CurrentUserId isn't valid. Perhaps we should just grab
CurrentUserId directly rather than going through
GetUserIdAndSecContext()? I could certainly do that instead.
Also includes those additional comments in elog.c.
Thanks,
Stephen
commit d9a7acd5ea1f5214b44875b6d257c5c59590167c
Author: Stephen Frost <sfrost@snowman.net>
Date: Wed Jan 12 12:53:50 2011 -0500
Use GetUserIdAndSecContext to get role OID in elog
We can't be sure that GetUserId() will be called when current_user
is a valid Oid, per the comments in GetUserIdAndSecContext, when
called from elog.c, so instead call GetUserIdAndSecContext directly
and handle the possible invalid Oid ourselves.
commit 605497b062298ea195d8999f8cefca10968ae22f
Author: Stephen Frost <sfrost@snowman.net>
Date: Wed Jan 12 12:29:44 2011 -0500
Change to logging role's OID instead of name
Remove the SysCache lookup from elog.c/log_line_prefix by logging
the role's OID instead, this addresses a concern where a SysCache
lookup could malfunction badly due to logging from a failed
transaction. Note that using SysCache from the elog routines could
also be a performance hit, though this would only be the case if a
user chose to enable that logging.
Attachments:
log_role_option.patchtext/x-diff; charset=us-asciiDownload+30-9
On Wed, Jan 12, 2011 at 12:59 PM, Stephen Frost <sfrost@snowman.net> wrote:
* Robert Haas (robertmhaas@gmail.com) wrote:
Logging the OID seems to be of questionable value.
I certainly disagree about this, not being able to figure out what's
causing a 'permissions denied' error because you don't know which role
the log is coming from is *very* annoying.
Interesting. I wonder if we shouldn't try to fix this by including
the relevant role name in the error message. Or is that just going to
be too messy to live?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
* Robert Haas (robertmhaas@gmail.com) wrote:
On Wed, Jan 12, 2011 at 12:59 PM, Stephen Frost <sfrost@snowman.net> wrote:
I certainly disagree about this, not being able to figure out what's
causing a 'permissions denied' error because you don't know which role
the log is coming from is *very* annoying.Interesting. I wonder if we shouldn't try to fix this by including
the relevant role name in the error message. Or is that just going to
be too messy to live?
It might be possible to do and answer that specific question- but what
about the obvious next question: which role was this command run with?
iow, if I log dml, how do I know what the role was when the dml
statement was run? ie- why was this command allowed?
Let's ask another question- why do we provide a %u option in
log_line_prefix instead of just logging it as part of each statement?
When you have roles that aren't 'inherit' and have a lot of 'set role's
happening, you end up asking the same questions about role that you
would about user.
As a side-note, CurrentUserId isn't actually exported (I'm not suprised,
tbh, but I've actually checked now), so you have to go through
GetUserIdAndSecContext().
Thanks,
Stephen
