FAST PostgreSQL wrote:
>> Am Dienstag, 3. April 2007 20:33 schrieb FAST PostgreSQL:
>>> Attached is the completed patch for the COPY-able sql log outputs.
>> Could you please remove random whitespace changes from this patch?
>
> Done and attached.

A couple of comments.

First one that's not at all about the code - but your emails appear to
be dated several days into the future. This one, for example, is
datestamped 2007-04-04. AFAIK timezones only go 13 (or is it 14?) hours
You'll want to look into that. (otherwise, it'll look like you missed
feature freeze..)


Looking at the code, I was first going to ask what happens when you
enable both redirect_stderr and this one - would the data be put in
random order in the same file. But looking at the code, it seems you're
appending ".sql" to the filename for this log. Two comments about that:

1) This is not mentioned anywhere in the docs or comments. It needs to be.

2) Not sure if .sql is such a great appendix to use, given that it's not
actual SQL. (I will also echo the other comment that was posted about
the whole name being sqllog, when it's not SQL. I think a better name is
needed since the data is csv. Goes for all parameters and function names)



Isn't the data included in the loglines in need of quoting/escaping?
What if there are weird charactersin the database name, for example? You
seem to only be escaping the error message itself. I think all
user-controlled data needs to be.


Then, it may be just me, but I find code like this:
! sqllogFile = fopen(strcat(filename, ".sql"), "a");

very hard to read. Took me a while to realize that's how it dealt with
the different filenames. I think it's better to do the filename catting
as a separate step.


//Magnus

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match

Magnus Hagander <magnus@hagander.net> writes:
> Then, it may be just me, but I find code like this:
> ! sqllogFile = fopen(strcat(filename, ".sql"), "a");
> very hard to read.

Such coding should be rejected out of hand anyway --- can you say
"buffer overrun"?

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

On Sat, 31 Mar 2007 14:09, you wrote:
> FAST PostgreSQL wrote:
> >> Am Dienstag, 3. April 2007 20:33 schrieb FAST PostgreSQL:
> >>> Attached is the completed patch for the COPY-able sql log outputs.
> >>
> >> Could you please remove random whitespace changes from this patch?
> >
> > Done and attached.
>
> A couple of comments.
>
> First one that's not at all about the code - but your emails appear to
> be dated several days into the future. This one, for example, is
> datestamped 2007-04-04. AFAIK timezones only go 13 (or is it 14?) hours
>
> You'll want to look into that. (otherwise, it'll look like you missed
>
> feature freeze..)

Sorry.. Fixed.

>
>
> Looking at the code, I was first going to ask what happens when you
> enable both redirect_stderr and this one - would the data be put in
> random order in the same file. But looking at the code, it seems you're
> appending ".sql" to the filename for this log. Two comments about that:
>
> 1) This is not mentioned anywhere in the docs or comments. It needs to be.

Agreed. As I mentioned in the previous mail, preferably alongwith the
recommended table structure to COPY the logs into. But where? Is adding a new
paragraph under the log_destination section of 'Where to Log' chapter ok ?

>
> 2) Not sure if .sql is such a great appendix to use, given that it's not
> actual SQL. (I will also echo the other comment that was posted about
> the whole name being sqllog, when it's not SQL. I think a better name is
> needed since the data is csv. Goes for all parameters and function names)

'csvlog' is another option. I can change that. Anyone have other preferences ?

>
>
>
> Isn't the data included in the loglines in need of quoting/escaping?
> What if there are weird charactersin the database name, for example? You
> seem to only be escaping the error message itself. I think all
> user-controlled data needs to be.
>
>
> Then, it may be just me, but I find code like this:
> ! sqllogFile = fopen(strcat(filename, ".sql"), "a");
>
> very hard to read. Took me a while to realize that's how it dealt with
> the different filenames. I think it's better to do the filename catting
> as a separate step.

I will fix that too.

Rgds,
Arul Shaji


>
>
> //Magnus


---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

On Mon, Apr 02, 2007 at 04:27:33PM +0000, FAST PostgreSQL wrote:
> On Sat, 31 Mar 2007 14:09, you wrote:
> > FAST PostgreSQL wrote:
> > >> Am Dienstag, 3. April 2007 20:33 schrieb FAST PostgreSQL:
> > >>> Attached is the completed patch for the COPY-able sql log outputs.
> > >>
> > >> Could you please remove random whitespace changes from this patch?
> > >
> > > Done and attached.
> >
> > A couple of comments.
> >
> > First one that's not at all about the code - but your emails appear to
> > be dated several days into the future. This one, for example, is
> > datestamped 2007-04-04. AFAIK timezones only go 13 (or is it 14?) hours
> >
> > You'll want to look into that. (otherwise, it'll look like you missed
> >
> > feature freeze..)
>
> Sorry.. Fixed.

Not quite. Time looks correct, but it specifies a timezone that's ten hours
off:
Received: from there (unknown 137.172.75.153)
by fast.fujitsu.com.au (Scalix SMTP Relay 10.0.1.3)
via ESMTP; Mon, 02 Apr 2007 16:28:34 +1000 (EST)
Date: Mon, 2 Apr 2007 16:27:33 +0000


> > Looking at the code, I was first going to ask what happens when you
> > enable both redirect_stderr and this one - would the data be put in
> > random order in the same file. But looking at the code, it seems you're
> > appending ".sql" to the filename for this log. Two comments about that:
> >
> > 1) This is not mentioned anywhere in the docs or comments. It needs to be.
>
> Agreed. As I mentioned in the previous mail, preferably alongwith the
> recommended table structure to COPY the logs into. But where? Is adding a new
> paragraph under the log_destination section of 'Where to Log' chapter ok ?

I think that makes sense. The comment in postgresql.conf might also need
some updating to reflect it.


> > 2) Not sure if .sql is such a great appendix to use, given that it's not
> > actual SQL. (I will also echo the other comment that was posted about
> > the whole name being sqllog, when it's not SQL. I think a better name is
> > needed since the data is csv. Goes for all parameters and function names)
>
> 'csvlog' is another option. I can change that. Anyone have other preferences ?

I assume csvlog for parameter names, and .csv as the file extension? Or did
you mean csvlog for extension as well? In that case, I'd suggest .csv ;-)

//Magnus


---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings