| From: | "FAST PostgreSQL" <fastpgs(at)fast(dot)fujitsu(dot)com(dot)au> |
|---|---|
| To: | pgsql-patches(at)postgresql(dot)org |
| Subject: | Re: COPY-able csv log outputs |
| Date: | 2007-04-18 01:23:11 |
| Message-ID: | 15633.14641176859445.fast.fujitsu.com.au@MHS |
| Views: | Raw Message | Whole Thread | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-patches |
[Modified subject line from sql to csv]
Attached is an updated patch for COPY-able log outputs. It incorporates all
the comments received on my previous patch including improved commenting,
documentation, escaping all user controlled data
Rgds,
Arul Shaji
On Wed, 4 Apr 2007 18:02, Russell Smith wrote:
> FAST PostgreSQL wrote:
> > On Wed, 4 Apr 2007 02:26, Andrew Dunstan wrote:
> >
> > I am currently doing the following modifications to the patch as per the
> > review comments.
> >
> > 1. Changing all references to 'sqllog' or 'sql' to 'csvlog' and 'csv'
> > respectively.
> > 2. Escaping the username and databasename
> > 3. Constructing the csvlog filename using log_filename instead of strcat.
> > 4. General code optimization.
> >
> > Any other changes required ? ?
>
> Function additions like get_timestamp need more comments, or better
> comments "returns the timestamp", which timestamp?
>
> > Will soon submit the updated patch.
> >
> > Rgds,
> > Arul Shaji
> >
> >> 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.
> >>
> >> Brief review of the CSV aspect only:
> >>
> >> a. username and databasename should probably be quoted and escaped in
> >> case they contain dangerous characters (even though that's unlikely)
> >> b. calling pg_get_client_encoding() inside the loop in
> >> escape_string_literal seems suboptimal. At the very least it should be
> >> moved outside the loop so it's only called once per string.
> >>
> >> cheers
> >>
> >> andrew
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 6: explain analyze is your friend
> <!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
> <html>
> <head>
> <meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
> </head>
> <body bgcolor="#ffffff" text="#000000">
> FAST PostgreSQL wrote:
> <blockquote cite="mid26121(dot)11031175649322(dot)fast(dot)fujitsu(dot)com(dot)au(at)MHS"
> type="cite">
> <pre wrap="">On Wed, 4 Apr 2007 02:26, Andrew Dunstan wrote:
>
> I am currently doing the following modifications to the patch as per the
> review comments.
>
> 1. Changing all references to 'sqllog' or 'sql' to 'csvlog' and 'csv'
> respectively.
> 2. Escaping the username and databasename
> 3. Constructing the csvlog filename using log_filename instead of strcat.
> 4. General code optimization.
>
> Any other changes required ? ?
> </pre>
> </blockquote>
> Function additions like get_timestamp need more comments, or better
> comments "returns the timestamp", which timestamp?<br>
> <br>
> <br>
> <blockquote cite="mid26121(dot)11031175649322(dot)fast(dot)fujitsu(dot)com(dot)au(at)MHS"
> type="cite">
> <pre wrap="">
> Will soon submit the updated patch.
>
> Rgds,
> Arul Shaji
>
>
> </pre>
> <blockquote type="cite">
> <pre wrap="">FAST PostgreSQL wrote:
> </pre>
> <blockquote type="cite">
> <blockquote type="cite">
> <pre wrap="">Am Dienstag, 3. April 2007 20:33 schrieb FAST
> PostgreSQL: </pre>
> <blockquote type="cite">
> <pre wrap="">Attached is the completed patch for the COPY-able
> sql log outputs. </pre>
> </blockquote>
> <pre wrap="">Could you please remove random whitespace changes from
> this patch? </pre>
> </blockquote>
> <pre wrap="">Done and attached.
> </pre>
> </blockquote>
> <pre wrap="">Brief review of the CSV aspect only:
>
> a. username and databasename should probably be quoted and escaped in
> case they contain dangerous characters (even though that's unlikely)
> b. calling pg_get_client_encoding() inside the loop in
> escape_string_literal seems suboptimal. At the very least it should be
> moved outside the loop so it's only called once per string.
>
> cheers
>
> andrew
> </pre>
> </blockquote>
> <pre wrap=""><!---->
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend
>
> </pre>
> </blockquote>
> <br>
> </body>
> </html>
| Attachment | Content-Type | Size |
|---|---|---|
| csvlog.patch | text/x-diff | 50.7 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Alvaro Herrera | 2007-04-18 01:45:27 | Re: log_autovacuum |
| Previous Message | Alvaro Herrera | 2007-04-17 22:57:49 | Re: log_autovacuum |