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. 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).
> 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.
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.
