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