Today we have two fairly common patterns around extracting an attr from a cached tuple:
a = SysCacheGetAttr(OID, tuple, Anum_pg_foo_bar, &isnull);
Assert(!isnull);
a = SysCacheGetAttr(OID, tuple, Anum_pg_foo_bar, &isnull);
if (isnull)
elog(ERROR, "..");
The error message in the elog() cases also vary quite a lot. I've been unable
to find much in terms of guidelines for when to use en elog or an Assert, with
the likelyhood of a NULL value seemingly being the guiding principle (but not
in all cases IIUC).
The attached refactoring introduce SysCacheGetAttrNotNull as a wrapper around
SysCacheGetAttr where a NULL value triggers an elog(). This removes a lot of
boilerplate error handling which IMO leads to increased readability as the
error handling *in these cases* don't add much (there are other cases where
checking isnull does a lot of valuable work of course). Personally I much
prefer the error-out automatically style of APIs like how palloc saves a ton of
checking the returned allocation for null, this aims at providing a similar
abstraction.
This will reduce granularity of error messages, and as the patch sits now it
does so a lot since the message is left to work on - I wanted to see if this
was at all seen as a net positive before spending time on that part. I chose
an elog since I as a user would prefer to hit an elog instead of a silent keep
going with an assert, this is of course debateable.
Thoughts?
--
Daniel Gustafsson
0001-Add-SysCacheGetAttrNotNull-for-guarnteed-not-null-at.patch
Description: Binary data
