Re: Refactoring SysCacheGetAttr to know when attr cannot be NULL

On Mon, Mar 13, 2023 at 02:19:07PM +0100, Daniel Gustafsson wrote:
> > On 2 Mar 2023, at 15:44, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> >> 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'd at least like to see it give the catalog's OID. That's easily
> > convertible to a name, and it doesn't tend to move around across PG
> > versions, neither of which are true for syscache IDs.
> >
> > Also, I'm fairly unconvinced that it's a "can't happen" --- this
> > would be very likely to fire as a result of catalog corruption,
> > so it would be good if it's at least minimally interpretable
> > by a non-expert. Given that we'll now have just one copy of the
> > code, ISTM there's a good case for doing the small extra work
> > to report catalog and column by name.
>
> Rebased v3 on top of recent conflicting ICU changes causing the patch to not
> apply anymore. Also took another look around the tree to see if there were
> missed callsites but found none new.

+++ b/src/backend/utils/cache/syscache.c
@@ -77,6 +77,7 @@
#include "catalog/pg_user_mapping.h"
#include "lib/qunique.h"
#include "utils/catcache.h"
+#include "utils/lsyscache.h"
#include "utils/rel.h"
#include "utils/syscache.h"

@@ -1099,6 +1100,32 @@ SysCacheGetAttr(int cacheId, HeapTuple tup,
+ elog(ERROR,
+ "unexpected NULL value in cached tuple for pg_catalog.%s.%s",
+ get_rel_name(cacheinfo[cacheId].reloid),

Question: Is it safe to be making catalog access inside an error
handler, when one of the most likely reason for hitting the error is
catalog corruption ?

Maybe the answer is that it's not "safe" but "safe enough" - IOW, if
you're willing to throw an assertion, it's good enough to try to show
the table name, and if the error report crashes the server, that's "not
much worse" than having Asserted().

--
Justin