On Mon, Mar 13, 2023 at 02:19:07PM +0100, Daniel Gustafsson wrote: > > On 2 Mar 2023, at 15:44, Tom Lane <t...@sss.pgh.pa.us> wrote: > > > > Peter Eisentraut <peter.eisentr...@enterprisedb.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