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.

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.]

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.


Russell Smith

---------------------------(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

Reply via email to