On 13.03.23 14:19, 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.
I think the only open question here was the granularity of the error
message, which I think you had addressed in v2.
+ if (isnull)
+ {
+ elog(ERROR,
+ "unexpected NULL value in cached tuple for
pg_catalog.%s.%s",
+ get_rel_name(cacheinfo[cacheId].reloid),
+ NameStr(TupleDescAttr(SysCache[cacheId]->cc_tupdesc,
attributeNumber - 1)->attname));
+ }
I prefer to use "null value" for SQL null values, and NULL for the C symbol.
I'm a bit hesitant about hardcoding pg_catalog here. That happens to be
true, of course, but isn't actually enforced, I think. I think that
could be left off. It's not like people will be confused about which
schema "pg_class.relname" is in.
Also, the cached tuple isn't really for the attribute, so maybe split
that up a bit, like
"unexpected null value in cached tuple for catalog %s column %s"
