Re: Add support for logging the current role - Mailing list pgsql-hackers

On Wed, Jan 19, 2011 at 14:36, Stephen Frost <sfrost@snowman.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
seemsto 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
descriptionfor 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
offile. 

* "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
rawstringand column_list in long-term context. Or, if you need TopMemoryContext, those variables should be pfree'ed at
theend 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
flattenedin 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