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 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? 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'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? 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. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers