Re: Add support for logging the current role

Itagaki,

* Itagaki Takahiro (itagaki(dot)takahiro(at)gmail(dot)com) wrote:
> ==== Discussions ====
> * How about "csvlog_fields" rather than "log_csv_fields"?
> Since we have variables with syslog_ prefix, csvlog_ prefix
> seems to be better.

Sure, not a big deal, to be honest, as long as we can actually agree on
something... Not changed in the patch, but I can if people want.

> * 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.

No, I don't, and that's going well beyond what I feel makes sense for
this patch at this time. We could review that for 9.2 or later, but
we've had quite enough expanding-of-requirements for this patch
already, imnsho.

> * 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.

Doing SIGHUP would require addressing how to get all of the backends to
close the old log file and open the new one, because we don't want to
have a given log file which has two different CSV formats in it (we
wouldn't be able to load it into the database...). This was
specifically addressed in the thread leading up to this patch...

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

none is what it is, however, if you query 'show role'. I would rather
be consistant with that than log something else.

> ==== 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.

You're right, rawstring and column_list don't need to be in
TopMemoryContext. I just moved the switch to Top to be after those are
allocated.

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

Urgh, right, fixed to *not* include a trailing comma on the last column.

> * 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.

Not sure what you're referring to here...? Can you elaborate? I'm not
great with the docs. :/

> * A declaration of assign_log_csv_fields() at the top of elog.c
> needs "extern".

Err, no, I don't think it does. None of the other exported functions
from elog.c have extern declarations in elog.c... I did realize that I
probably shouldnt have the declaration at the top of elog.c for
assign_loc_csv_fields() or build_default_csvlog_list().

> * There is a duplicated declaration for build_default_csvlog_list().

Removed the duplicate in elog.c.

> * list_free() is NIL-safe. You don't have to check whether the list
> is NIL before call the function.

Fixed.

Updated patch attached.

Thanks!

Stephen