Re: COPY-able sql log outputs - Mailing list pgsql-patches

FAST PostgreSQL wrote:

I have attempted to do a quick review of the code in this patch, I've
not installed or compiled it.  However I do have a couple of comments.
> Hi,
>
> Attached is the completed patch for the COPY-able sql log outputs. I have
> modified my previous WIP patch, incorporating all the changes requested by
> the community.  This patch has been tested both on windows and linux.
>
> Reiterating what has been done.
>
> The log is now output in COPY-able format as suggested. (Not INSERT-able as
> was in the previous WIP patch.)
>
> log_destination now accepts 'sqllog' as a valid output destination. The log
> output file will be determined by pg_log and log_filename variables. The sql
> log output filename will be 'log_filename'.sql. The file rotation rules apply
> to the sql log file output as well.
>
I'm not sure I'm sold on sqllog given it's not actually sql at all, it's
just a CSV/text file.  But I don't have any better names at this time.
copylog, justifiedlog, csvlog  come to mind, but that all seem bad.
> The log output format is as follows.
>
> timestamp, username, database_name, sessionid, host_port, process_id,
> command_tag, session_start_time, transaction_id, error_severity,
> sql_state_code, statement
>
> The logs can be loaded into a table using the command
>
> COPY sqltable FROM 'filename.sql' WITH CSV;
>
I am concerned with the fact that no escaping is attempted on anything
except the error message itself, is it known that it's not possible to
have a ',' in the rest of the field types?
> There are only two minor issues I can think of
> 1. The sql log is currently output with newline and tab characters. It loads
> into the table neatly. No problems. But when read back, atleast from psql in
> windows, the tabs are replaced with some special characters.
>
> 2. I think it is better to document somewhere the table structure and the
> COPY statement above. But where?
>
With regard to where to document the column structure, how about a
header line when a new file is opened, that will allow users to use
whatever table structure they like, and can import relevant columns from
the log file.  You should also mention the format in the logging section
under the sqllog parameter in the docs.
> [P.S. - In the wake of community's concerns regarding the legal disclaimer
> getting attached to the end of mails we send to the community, we have got an
> exemption from the disclaimer getting attached. As this is the first mail I
> am sending after this approval, fingers crossed, it works. If for some reason
> it gets attached, please ignore this mail and I will send the patch from some
> other account.]
>
> Rgds,
> Arul Shaji
>
Other Patch comments;

Is there an overall line length you should be adhering too?  I think
there is, so you will want to shorten those lines.

+         /* Win32 timezone names are too long so don't print them */
+ #ifndef WIN32
+              "%Y-%m-%d %H:%M:%S %Z",
+ #else
+              "%Y-%m-%d %H:%M:%S",
+ #endif

Do we actually care that the WIN32 format is long for this type of log?



+ /*
+  * Calculates and returns the timestamp
+  */
+ static void
+ get_timestamp(StringInfo buf)

"the timestamp" some words about the fact it gets the "current"
timestamp might be helpful an that it appends to a string.

!         sqllogFile = fopen(strcat(filename, ".sql"), "a");

I don't believe forcing .sql for a CSV file is a good idea, let the user
decide what they want to call it, if you are forcing anything, at least
make it ".csv".


+ #define LOG_DESTINATION_SQL    8

SQLLOG I would think for completness and consistency of naming.


Hope this helps get your patch into PostgreSQL cleanly and quickly.

Regards

Russell Smith