> On 2 Mar 2023, at 10:59, Peter Eisentraut <peter.eisentr...@enterprisedb.com> > wrote: > > On 28.02.23 21:14, Daniel Gustafsson wrote: >> The attached refactoring introduce SysCacheGetAttrNotNull as a wrapper around >> SysCacheGetAttr where a NULL value triggers an elog(). This removes a lot of >> boilerplate error handling which IMO leads to increased readability as the >> error handling *in these cases* don't add much (there are other cases where >> checking isnull does a lot of valuable work of course). Personally I much >> prefer the error-out automatically style of APIs like how palloc saves a ton >> of >> checking the returned allocation for null, this aims at providing a similar >> abstraction. > > I looked through the patch.
Thanks! > The changes look ok to me. In some cases, more line breaks could be removed > (that is, the whole call could be put on one line now). I've tried to find those that would fit on a single line in the attached v2. >> This will reduce granularity of error messages, and as the patch sits now it >> does so a lot since the message is left to work on - I wanted to see if this >> was at all seen as a net positive before spending time on that part. I chose >> an elog since I as a user would prefer to hit an elog instead of a silent >> keep >> going with an assert, this is of course debateable. > > I think an error message like > > "unexpected null value in system cache %d column %d" > > is sufficient. Since these are "can't happen" errors, we don't need to spend > too much extra effort to make it prettier. They really should never happen, but since we have all the information we need it seems reasonable to ease debugging. I've made a slightly extended elog in the attached patch. Callsites which had a detailed errormessage have been left passing isnull, like for example statext_expressions_load(). > I don't think the unlikely() is going to buy much. If you are worried on > that level, SysCacheGetAttrNotNull() ought to be made inline. Looking through > the sites of the changes, I didn't find any callers where I'd be worried on > that level. Fair enough, removed. -- Daniel Gustafsson
v2-0001-Add-SysCacheGetAttrNotNull-for-guarnteed-not-null.patch
Description: Binary data
