Re: Avoid possible dereference null pointer (src/backend/utils/cache/relcache.c)

On 6/17/25 15:09, Aleksander Alekseev wrote:
> Hi,
> 
>>> We would be in serious trouble if RelationReloadNailed() could be
>>> called for a non-existing relation. Wouldn't Assert() be more
>>> appropriate?
>>
>> IMO, I think no.
>>
>> In all paths, this check is done, why would this be the only exception?
> 
> We use Asserts() for cases that shouldn't happen in practice for
> performance reasons. Since this code doesn't crash I suspect this is
> one of such cases. Unless you are aware of a specific scenario that
> makes the code crash of course.
> 

Right. Assert() are for checks that we don't expect to ever fail, so we
don't want to keep them in production builds. I believe this is exactly
such case, because the code is about "nailed" catalogs, that we need to
access very early (e.g. before being able to access pg_class).

There's only ~10 of such catalogs (look for formrdesc calls), and if we
get a failure here, it's not clear to me we can really keep the system
running anyway. It's not just the usual "relcache miss" and if the user
retries it will probably work fine. The catalog is borked, and who knows
in what way.

My opinion is that adding a "elog(ERROR)" here would be misleading, as
it implies it's something we expect. And mostly pointless. I can imagine
adding an Assert, but I don't quite see how is that better than just
hitting a segfault a couple lines later.


regards

-- 
Tomas Vondra