Re: Refactoring SysCacheGetAttr to know when attr cannot be NULL

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"