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

* Robert Haas (robertmhaas@gmail.com) wrote:
> 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.

Will fix.

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

I couldn't make it actually produce incorrect lines.  I was worried
about the possibility, but I don't think it's actually possible because
postgresql.conf needs to be parsed and GUC handling has to work before
we will even start trying to do CSV logging.  I'll rework the comment.

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

Sure, will fix.

> 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?

As I recall, Tom didn't like that idea and neither did I- there's only
so many letters..  It also sucks wrt being self-documenting.  I'd much
rather support multi-line GUCs..

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

This would be called a 'header' in most typical CSV scenarios.
Unfortunately, last I checked (maybe it's changed?), COPY w/ HEADER just
throws the header away instead of doing anything useful with it.  If it
actually used the header to build the column list, then adding a header
would be sufficient, provided all the necessary fields are in the table.

If I wanted something to throw away the first record of a file before
loading it, I'd use tail.

I'll see about adding an option to have it output a header.
Thanks,
    Stephen