Re: Add support for logging the current role

On Fri, Feb 11, 2011 at 11:23 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Updated patch attached, full git log below.

The documentation for csvlog_fields should probably use <literal>
around the default value.

The sentence that begins "For details on what these fields are" should
hyperlink the referenced sections of the documentation.

The function prototype you added to elog.c is misformatted - the type
should be on the line preceding the function name only for the
definition, not for prototypes.

The code for log_line_prefix = 'u' is indented wrong. Also, an else
clause that has only an if hanging off of it can be turned into an
"else if" for better readability.

This part kind of concerns me:

+ * This is more of a 'safety valve' than anything else,
+ * since GUC processing really should happen before we do any error logging.
+ * We might even want to change this eventually to just not log CSV format logs
+ * if this ever happens, to avoid a discrepency in the CSV log file which would
+ * make it difficult to load into PG.

I'm not really convinced that making the CSV log format variable is a
good thing. One of the reasons we added that format in the first
place is to make sure that we could generate log output in an easily
parseable format, and this seems like a big step backwards for not
much, especially if we can't even guarantee that we're not going to
inject random differently-formatted lines during startup.

Stylistically, build_default_csvlog_list and assign_csvlog_fields
ought to be loops driven off an array, rather than hand-coded.

I think this was discussed before, and I hate to remention it, but
would it make sense to reuse the single-character codes from
log_line_prefix rather than inventing new codes for the same fields?

It would be awfully nice if we could inject something into the csvlog
output format that would let client programs know which fields are
present. This would be especially useful for people writing tools
that are intended to work on ANY PG installation, rather than just,
say, their own. I'm not sure if there's a clean way to do that,
though.

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