On 13.03.23 14:19, Daniel Gustafsson wrote:
>> On 2 Mar 2023, at 15:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Peter Eisentraut <peter.eisentraut@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"
