Patch review for logging hooks (CF 2012-01) - Mailing list pgsql-hackers

Hi,

Here's my review for the "logging hooks" patch queued for the 2012-01
CommitFest by Martin Pihlak.

Submission review
----
The patch is in context diff format and applies fine. Tests are not
included and don't seem practical for this patch.

More documentation would always be nice, but as other hooks aren't
documented either, it seems that's acceptable. The function prototype
and ErrorData structure are largely self-documenting.

Usability review
----
> Do we want that?
> Do we already have it?

The patch aims to allow custom processing of log messages. There are
two slightly overlapping features already in Postgres: csvlog and
syslog. Both seem to have their drawbacks; csvlog is written to disk
first, and has to be processed in the background in batches. Syslog is
pretty universal, but the output format is more limited, and details
of a single report are split up into several lines. Also passing extra
data for parsing via log_line_prefix seems hacky and failure prone.

The proposal also allows more flexible filtering/instrumentation of
log messages, which is not available with current methods.

> Does the patch actually implement that?

The hooked EmitErrorReport function is responsible for calling other
log handlers, so all relevant logging and context information is
available.

> Does it follow SQL spec, or the community-agreed behavior?

So far, a few people have stated that this sort of a logging hook is
desirable. Nobody has opposed it so far.

Feature test
----
I tested the hook using Martin's own pg_logforward extension:
https://github.com/mpihlak/pg_logforward

I verified that the hook works for messages with DEBUG, LOG, NOTICE,
ERROR and FATAL levels, from the backend as well as postmaster.

> Are there corner cases the author has failed to consider?

Whether the hook is called depends on both the 'client_min_messages'
and 'log_min_messages' settings because of this optimization in
errstart:/* Skip processing effort if non-error message will not be output */if (elevel < ERROR && !output_to_server &&
!output_to_client)   return false;
 

This will certainly be surprising to users. I think making it depend
*only* on output_to_server (and thus log_min_messages) would be more
predictable.

Coding review
----
> Does it follow the project coding guidelines?

There's a minor whitespace problem. When declaring variables, and the
data type is longer than 12 characters, just use 1 single space
character to delimit the variable name, instead of tab:

extern emit_log_hook_type    emit_log_hook;

> Will it work on Windows/BSD etc?

I see that other hooks are declared with PGDLLIMPORT. I presume that
this is necessary on Windows:
extern PGDLLIMPORT emit_log_hook_type emit_log_hook;

> Are the comments sufficient and accurate?

I think the hook warrants a comment that, whether the messages will be
seen, depends on the log_min_messages setting.

----

PS: This is my first patch review ever, any feedback would be welcome.

Regards,
Marti