On Wed, Jan 19, 2011 at 14:36, Stephen Frost <sfr...@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 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
