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

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Fri, Feb 11, 2011 at 11:23 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > Updated patch attached, full git log below.

Thanks again for the review.  Updated patch attached.

> The documentation for csvlog_fields should probably use <literal>
> around the default value.

Fixed.

> The sentence that begins "For details on what these fields are" should
> hyperlink the referenced sections of the documentation.

Fixed.

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

Fixed.

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

Fixed.

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

Comment, function, and whole issue removed.

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

Done, added CSVFieldNames and modiffied assign_csvlog_fileds to use it
(build_default_csvlog_list was removed).

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

I'd rather ditch the log_line_prefix idea of single-letter codes since
it's never going to scale.  Perhaps making log_line_prefix with work
%csvlog_name instead of just the %<single-letter> codes might work.  I
don't see a solution which doesn't involve changing log_line_prefix
though, in any case.

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

Added csvlog_header GUC to allow the user to ask for the header to be
printed at the top of each log file.  If and when an option is added to
PG's COPY to respect the header, this should resolve that issue.

Also updated to HEAD.

Full git log below, patch attached.

    Thanks,

        Stephen

commit 592c256ffff4ffde77fc29ff28fdedd2c9f2dafd
Merge: 33639eb cebbaa1
Author: Stephen Frost <sfrost@snowman.net>
Date:   Sun Feb 13 21:11:44 2011 -0500

    Merge branch 'master' of git://git.postgresql.org/git/postgresql into log_csv_options

commit 33639ebfe67b0dd58a0a89161e9f0d5237830ed4
Author: Stephen Frost <sfrost@snowman.net>
Date:   Sun Feb 13 21:08:08 2011 -0500

    Add csvlog_header GUC, other cleanup

    This patch adds a csvlog_header option which will start each CSV
    log file with a header which matches the GUC (and hence the format
    of the CSV log file generated).

    Numerous other whitespace clean-ups, removed build_default_csvlog_list(),
    since it wasn't actually necessary or useful.  Added an array which
    lists the text strings of the various CSVLOG options to simplify
    assign_csvlog_fields().

commit 6bd2b9f1d2bc3b166a3e5598ee590e25159c61a5
Author: Stephen Frost <sfrost@snowman.net>
Date:   Fri Feb 11 11:16:17 2011 -0500

    Rename log_csv_fields GUC to csvlog_fields

    This patch renames the log_csv_fileds GUC to csvlog_fields, to better
    match the other csvlog_* options.

    Also cleaned up the CSV generation code a bit by moving the comma-adding
    code out of the switch() statement.

commit a281ca611e6181339e92b488c815e0cb8c1298d2
Merge: d8dddd1 183d3cf
Author: Stephen Frost <sfrost@snowman.net>
Date:   Fri Feb 11 08:37:27 2011 -0500

    Merge branch 'master' of git://git.postgresql.org/git/postgresql into log_csv_options

commit d8dddd1c425a4c320540769084ceeb7d23bc3662
Author: Stephen Frost <sfrost@snowman.net>
Date:   Sun Feb 6 14:02:05 2011 -0500

    Change log_csv_options listing to a table

    This patch changes the listing of field options available to
    log_csv_options into a table, which will hopefully both look
    better and be clearer.

commit f9851cdfaeb931f01c015f5651b72d16957c7114
Merge: 3e71e33 5ed45ac
Author: Stephen Frost <sfrost@snowman.net>
Date:   Sun Feb 6 13:26:17 2011 -0500

    Merge branch 'master' of git://git.postgresql.org/git/postgresql into log_csv_options

commit 3e71e338a2b9352d730f59a989027e33d99bea50
Author: Stephen Frost <sfrost@snowman.net>
Date:   Fri Jan 28 22:44:33 2011 -0500

    Cleanup log_csv_options patch

    Clean up of various function declarations to hopefully be correct
    and clean and matching PG conventions.  Also move TopMemoryContext
    usage to later, since the local variables don't need to be in
    TopMemoryContext.  Lastly, ensure that a comma is not produced
    after the last CSV field, and that one is produced if
    application_name is not the last field.

    Review by Itagaki Takahiro, thanks!

commit 1825def11badd661d219fa4c516f06e0ad423443
Merge: ff249ae 847e8c7
Author: Stephen Frost <sfrost@snowman.net>
Date:   Wed Jan 19 06:50:03 2011 -0500

    Merge branch 'master' of git://git.postgresql.org/git/postgresql into log_csv_options

commit ff249aeac7216da623bf77840380d5e767f681fc
Author: Stephen Frost <sfrost@snowman.net>
Date:   Wed Jan 19 00:26:52 2011 -0500

    Add log_csv_fields GUC for CSV output & curr_role

    This patch adds a new GUC called 'log_csv_fields'.  This GUC allows
    the user to control the set of fields written to the CSV output as
    well as the order in which they are written.  The default set of
    fields remains those that were included in 9.0, to avoid breaking
    existing user configurations.

    In passing, update 'user_name' for log_line_prefix and log_csv_fields
    to mean 'session user' (which could be reset by a superuser with
    set session authorization), and add a 'role_name' option (%U) to
    log_line_prefix and log_csv_fields, to allow users to log the
    current role (as set by SET ROLE- not impacted by SECURITY DEFINER
    functions).