Re: Add support for logging the current role

On Wed, Jan 19, 2011 at 14:36, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Alright, here's the latest on this patch.  I've added a log_csv_fields
> GUC along with the associated magic to make it work (at least for me).
> Also added 'role_name' and '%U' options.  Requires postmaster restart to
> change, didn't include any 'short-cut' field options, though I don't
> think it'd be hard to do if we can decide on it.  Default remains the
> same as what was in 9.0.

Hi, I reviewed log_csv_options.patch. It roughly looks good,
but I'd like to discuss about the design in some points.

==== Features ====
The patch adds "log_csv_fields" GUC variable. It allows to customize
columns in csvlog. The default setting is what we were writing 9.0 or
earlier versions.

It also add "role_name" for log_csv_fields and "%U" for log_line_prefix
to record role names. They are the name set by SET ROLE. OTOH, user_name
and %u shows authorization user names.

==== Discussions ====
* How about "csvlog_fields" rather than "log_csv_fields"?
Since we have variables with syslog_ prefix, csvlog_ prefix
seems to be better.

* We use %<what> syntax to specify fields in logs for log_line_prefix,
but will use long field names for log_csv_fields. Do you have any
better idea to share the names in both options? At least I want to
share the short description for them in postgresql.conf.

* log_csv_fields's GUC context is PGC_POSTMASTER. Is it by design?
PGC_SIGHUP would be more consistent compared with log_line_prefix.
However, the csv format will not be valid because the column
definitions will be changed in the middle of file.

* "none" is not so useful for the initial "role_name" field.
Should we use user_name instead of the empty role_name?

==== Comments for codes ====
Some of the items are trivial, though.

* What objects do you want to allocate in TopMemoryContext in
assign_log_csv_fields() ? AFAICS, we don't have to keep rawstring
and column_list in long-term context. Or, if you need TopMemoryContext,
those variables should be pfree'ed at the end of function.

* appendStringInfoChar() calls in write_csvlog() seem to be wrong
when the last field is not application_name.

* Docs need more cross-reference hyper-links for "see also" items.

* Docs need some tags for itemized elements or pre-formatted codes.
They looks itemized in the sgml files, but will be flattened in
complied HTML files.

* A declaration of assign_log_csv_fields() at the top of elog.c
needs "extern".
* There is a duplicated declaration for build_default_csvlog_list().
* list_free() is NIL-safe. You don't have to check whether the list
is NIL before call the function.

--
Itagaki Takahiro