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

Em ter., 17 de jun. de 2025 às 10:43, Tomas Vondra <tomas(at)vondra(dot)me>
escreveu:

> 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.
>
To me this is a contradiction, whether you consider waiting for a segfault
or consider adding an Assert.
For the user it is better to have a log, where he can quickly find the
problem, rather than having to investigate on his own.

best regards,
Ranier Vilela