Re: add support for logging current role (what to review?)

Ive been holding off because its marked as Waiting on Author, am now
thinking thats a mistake. =)

It links to this patch:
http://archives.postgresql.org/message-id/20110215135131.GX4116@tamriel.snowman.net

Which is older than the latest patch in that thread posted by Robert:
http://archives.postgresql.org/message-id/AANLkTikMadttguOWTkKLtgfe90kxR=U9njk9zEbRwb-+@mail.gmail.com

(There are also various other patches and versions in that thread...)

The main difference between the first and the last patch is the first
one has support for changing what csv columns we output, while the
latter just tacks on an additional column.

The thread was very long and left me a bit confused as to what I
should actually be looking at. Or perhaps thats the point-- we need to
decide if a csvlog_fields GUC is worth it.

On Mon, Jun 27, 2011 at 6:25 PM, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
> Ive been holding off because its marked as Waiting on Author, am now
> thinking thats a mistake. =)
>
> It links to this patch:
> http://archives.postgresql.org/message-id/20110215135131.GX4116@tamriel.snowman.net
>
> Which is older than the latest patch in that thread posted by Robert:
> http://archives.postgresql.org/message-id/AANLkTikMadttguOWTkKLtgfe90kxR=U9njk9zEbRwb-+@mail.gmail.com
>
> (There are also various other patches and versions in that thread...)
>
> The main difference between the first and the last patch is the first
> one has support for changing what csv columns we output, while the
> latter just tacks on an additional column.
>
> The thread was very long and left me a bit confused as to what I
> should actually be looking at. Or perhaps thats the point-- we need to
> decide if a csvlog_fields GUC is worth it.

This is pretty much a classic example of the tailing wagging the dog.

Adding a new field to the output would be pretty easy if it weren't
for the fact that we want the CSV log output to contain all the same
fields that are available in the regular log. Of course, adding a
field to the CSV format is no big deal either. But now you have more
problems: (1) it breaks backward compatibility, (2) it makes the log
files bigger, and (3) if the new field is more expensive to compute
than the existing ones, then you're forcing people who want to use the
CSV log format to incur an overhead for a feature they don't care
about. We could surmount these problems by making the CSV log format
more flexible, but (a) that's a lot of work, (b) it imposes a burden
on tool-builders, and (c) it might produce a really long icky-looking
configuration line in postgresql.conf to show the default value of the
GUC controlling the log format. (You may think I'm joking about this
last one, but the point was raised... several times.)

The upshot, presumably, is that if Stephen would like us to add this
new field, he needs to be willing to rewrite our entire logging
infrastructure first... and then maybe we'll bounce the rewrite
because it adds too much complexity, but who was it that asked for all
that complexity again? Oh, that's right: we did. Now, I admit that
I've been guilty of engaging in this kind of scope creep from time to
time myself, so everyone feel free to pitch your favorite loose object
in my direction.

Anyway, if we are going to insist on rewriting substantial chunks of
the logging infrastructure before doing this, we at least need to
reach some agreement on what would be an acceptable outcome - and then
let Stephen code that up as a separate patch, submit it, get it
committed, and then do this on top of that. Or we can just decide
that adding one relatively minor field to the text format output is
not worth knocking over the applecart for.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jun 28, 2011 at 09:15, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Anyway, if we are going to insist on rewriting substantial chunks of
> the logging infrastructure before doing this, we at least need to
> reach some agreement on what would be an acceptable outcome - and then
> let Stephen code that up as a separate patch, submit it, get it
> committed, and then do this on top of that.  Or we can just decide
> that adding one relatively minor field to the text format output is
> not worth knocking over the applecart for.

Well, one of the reasons I picked up this patch is its a feature *I*
want. I remember being quite surprised at the role csv logging reports
when i switched to it. I was logging everything so I have on occasion
had to find a SET ROLE and follow the session... its quite annoying
:-)

I think if Stephen was proposing 10 fields, or if there was a list of
fields we were planning on adding in the next release or 3, it might
be worth re-factoring. I know of no such list, and I think this field
useful/important enough that people who are using csv logging would
want it anyway. +1 on just tacking on the field and causing a flag day
for csv users.

* Alex Hunsaker (badalex(at)gmail(dot)com) wrote:
> I think if Stephen was proposing 10 fields, or if there was a list of
> fields we were planning on adding in the next release or 3, it might
> be worth re-factoring.

I know of at least one person (in an earlier piece of the thread
discussing this patch) who was talking about other fields they'd like
included in the CSV log which aren't currently. I don't recall what
that was though, but I think it might have been something like line #
from inside stored procedures..

> I know of no such list, and I think this field
> useful/important enough that people who are using csv logging would
> want it anyway. +1 on just tacking on the field and causing a flag day
> for csv users.

Honestly, I think it was *me* who raised the issue that we don't have a
header for CSV logs and that it sucks for people using CSV files. We've
changed it in the past (application_name was added, iirc) and there
wasn't much noise of it that I recall. If everyone's happy with that,
it's fine by me.

I do want to rework the logging infrastructure (as discussed in the dev
meeting), but I see that whole thing as rather orthogonal to this
change.

Thanks,

Stephen

Excerpts from Stephen Frost's message of jue jun 30 18:35:40 -0400 2011:

> > I know of no such list, and I think this field
> > useful/important enough that people who are using csv logging would
> > want it anyway. +1 on just tacking on the field and causing a flag day
> > for csv users.
>
> Honestly, I think it was *me* who raised the issue that we don't have a
> header for CSV logs and that it sucks for people using CSV files. We've
> changed it in the past (application_name was added, iirc) and there
> wasn't much noise of it that I recall. If everyone's happy with that,
> it's fine by me.

I don't understand why it is so much a deal that 9.1 has a different CSV
table definition than 9.0 anyway (or any two release combination). As
long as both are clearly and correctly documented in the respective
pages, it shouldn't be an issue at all. If anyone attempts to load CSV
log files into the wrong table definition, the problem should make
itself evident pretty quickly.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Fri, Jul 1, 2011 at 03:36, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Stephen Frost's message of jue jun 30 18:35:40 -0400 2011:
>
>> > I know of no such list, and I think this field
>> > useful/important enough that people who are using csv logging would
>> > want it anyway. +1 on just tacking on the field and causing a flag day
>> > for csv users.
>>
>> Honestly, I think it was *me* who raised the issue that we don't have a
>> header for CSV logs and that it sucks for people using CSV files.  We've
>> changed it in the past (application_name was added, iirc) and there
>> wasn't much noise of it that I recall.  If everyone's happy with that,
>> it's fine by me.
>
> I don't understand why it is so much a deal that 9.1 has a different CSV
> table definition than 9.0 anyway (or any two release combination).  As
> long as both are clearly and correctly documented in the respective
> pages, it shouldn't be an issue at all.  If anyone attempts to load CSV
> log files into the wrong table definition, the problem should make
> itself evident pretty quickly.

I don't see that as a big problem either. A file header would make it
easier to detect for tools though - either by a regular CSV header or
by just always logging a row with the version number in the first line
of each file. Making it an actual CSV header has the added benefit of
being a standard way that non-pg-specific tools know how to deal
with..

If it's a pg specific tool, it's likely going to require version
specific information anyway. This is just one of them. Now, if the
order and ocntents of the fields is made entirely configurable, that
basically creates an unlimited number of permutations, and *that* may
make things really hard on tool developers. And without a header, it
makes the tool developers work completely impossible - but harder even
with.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Stephen Frost <sfrost(at)snowman(dot)net> writes:

> * Alex Hunsaker (badalex(at)gmail(dot)com) wrote:
>> I think if Stephen was proposing 10 fields, or if there was a list of
>> fields we were planning on adding in the next release or 3, it might
>> be worth re-factoring.
>
> I know of at least one person (in an earlier piece of the thread
> discussing this patch) who was talking about other fields they'd like
> included in the CSV log which aren't currently. I don't recall what
> that was though, but I think it might have been something like line #
> from inside stored procedures..
>

well, while not in this thread i have been advocating that we should have a
column for duration... that way pg tools won't need to look for
"duration" (which makes those tools useless in non-english
installations)

--
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL
Soporte 24x7, desarrollo, capacitación y servicios

On Thu, Jun 30, 2011 at 16:35, Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> I do want to rework the logging infrastructure (as discussed in the dev
> meeting), but I see that whole thing as rather orthogonal to this
> change.

*Shrug* Fine by me, im not going to argue that you should or shouldn't
rework the logging infrastructure. There seem to be people on both
sides of the fence... Including me. Im of the opinion that for *this*
field we should just add it, I think it stands on its own legs. For
some of the other columns (duration, pl line # etc)... well It might
make more sense to have a knob.

So uhh.. Im still confused at exactly what patch I should be looking
at because of my opinion that we should just add this field.

Stephen (others?), whats your preference? Maybe this should be two
patches (one that adds the field, another that adds a header)?

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Fri, Jul 1, 2011 at 03:36, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
>> I don't understand why it is so much a deal that 9.1 has a different CSV
>> table definition than 9.0 anyway (or any two release combination).

> I don't see that as a big problem either. A file header would make it
> easier to detect for tools though - either by a regular CSV header or
> by just always logging a row with the version number in the first line
> of each file. Making it an actual CSV header has the added benefit of
> being a standard way that non-pg-specific tools know how to deal
> with..

> If it's a pg specific tool, it's likely going to require version
> specific information anyway. This is just one of them. Now, if the
> order and ocntents of the fields is made entirely configurable, that
> basically creates an unlimited number of permutations, and *that* may
> make things really hard on tool developers. And without a header, it
> makes the tool developers work completely impossible - but harder even
> with.

Actually, I thought the argument for a configurable column list was the
other way around. Somebody who wanted to use a tool that only
understood, say, the 9.0 CSV column set could configure his 9.2 database
to output that set, regardless of whether we'd added other column types.

Of course, there are less-complicated ways to provide backwards
compatibility than making the column set fully configurable --- but
providing the feature that way would have other benefits, namely not
having to waste log space and computation effort on columns that a
particular DBA doesn't care about.

No objection here to the idea of adding a CSV header line; just want to
say that there are real reasons to want a configurable column set.

regards, tom lane