Re: Refactoring SysCacheGetAttr to know when attr cannot be NULL

> On 2 Mar 2023, at 10:59, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)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