17.01.2014 00:13, Tom Lane kirjoitti:
Oskari Saarenmaa <o...@ohmu.fi> writes:
[ 0001-Filter-error-log-statements-by-sqlstate.patch ]
I looked at this patch. It took me some time to understand that what
it actually does has got approximately nothing to do with what one might
first expect: rather than suppressing the entire log message about some
error, it only suppresses printing of the triggering SQL statement
(if that's available to begin with). The proposed documentation is
certainly not clear enough on that point, and the API which appears to
allow changing the error severity level associated with a SQLSTATE isn't
exactly helping to clarify things either.
Also, the patch claims it allows logging of statements that otherwise
wouldn't get logged, but AFAICS that's wrong, because we'll never get to
this code at all if errstart decides we're not going to log the message.
I agree the documentation (and perhaps the feature itself) are a bit
confusing, but the idea is to control SQL statement logging when errors
occur. This patch doesn't do anything about normal error logging, it
only controls when the statements are printed.
Running:
set log_min_messages = 'warning';
set log_min_error_statement = 'panic';
set log_sqlstate_error_statement = '';
do 'begin raise exception ''foobar 1''; end';
set log_sqlstate_error_statement = 'P0001:error';
do 'begin raise exception ''foobar 2''; end';
set log_min_error_statement = 'error';
set log_sqlstate_error_statement = 'P0001:panic';
do 'begin raise exception ''foobar 3''; end';
logs
2014-01-17 00:37:12 EET ERROR: foobar 1
2014-01-17 00:37:20 EET ERROR: foobar 2
2014-01-17 00:37:20 EET STATEMENT: do 'begin raise exception
''foobar 2''; end';
2014-01-17 00:38:34 EET ERROR: foobar 3
I find it hard to follow exactly what the use-case for this is; could
you make a defense of why we should carry a feature like this?
I usually run PG with log_min_messages = warning and
log_min_error_statement = error to log any unexpected errors. But as I
have a lot of check constraints in my database as well as a lot of
plpgsql and plpython code which raises exceptions on invalid user input
I also get tons of logs for statements causing "expected" errors which I
have to try to filter out with other tools.
In any case, I'm finding it hard to believe that filtering by individual
SQLSTATEs is a practical API. When we've discussed providing better log
filtering in the past, that approach was invariably dismissed on the
grounds that it would be far too unwieldy to use --- any DBA attempting to
use it in anger would end up with a huge and ever-incomplete list of
SQLSTATEs he'd need to filter. A long time ago I suggested that filtering
by SQLSTATE class (the first two characters of SQLSTATE) might be useful,
but I'm not sure I still believe that, and anyway it's not what's
implemented here.
I don't know about others, but filtering by individual SQLSTATE is
exactly what I need - I don't want to suppress all plpgsql errors or
constraint violations, but I may want to suppress plpgsql RAISE
EXCEPTION and CHECK violations.
I'm concerned also about the potential performance impact of this patch,
if used with a SQLSTATE list as long as I think they're likely to get in
practice. Have you done any performance testing?
Not yet. As this only applies to statement logging (for now at least) I
doubt it's a big deal, formatting and writing the statement somewhere is
probably at least as expensive as looking up the configuration.
As far as the code goes, bit manipulations on uint64s are a pretty crummy
substitute for defining a struct with a couple of fields; and the way
you're choosing to sort the array seems mighty inefficient, as well as
probably wrong in detail (why is the loop ignoring the first array
element?); and rather than make fragile assumptions about the maximum
length of an elevel name, why not just leave the user's string as-is?
But I wouldn't advise worrying about code style issues until we decide if
we're accepting the feature. Right now my inclination is to reject it.
I agree, I should've just defined a struct and used the original string
length when rewriting user string (it's rewritten to drop any
duplicates.) I don't think the sort is a big deal, it's only done when
the value is defined; the first array element is the length of the
array. I can address these points in a new version of this patch if the
feature looks useful.
Thanks for the review!
Oskari
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
