Thread: 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@gmail.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@gmail.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@gmail.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@commandprompt.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@commandprompt.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@snowman.net> writes: > * Alex Hunsaker (badalex@gmail.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@snowman.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@hagander.net> writes:
> On Fri, Jul 1, 2011 at 03:36, Alvaro Herrera <alvherre@commandprompt.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