Re: [PATCH] Filter error log statements by sqlstate

17.01.2014 00:13, Tom Lane kirjoitti:
> Oskari Saarenmaa <os@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
''foobar3''; 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