Обсуждение: using SysCacheGetAttrNotNull in place of heap_getattr

I was about to push a quick patch to replace the use of heap_getattr()
in get_primary_key_attnos() with SysCacheGetAttrNotNull(), because that
makes the code a few lines shorter and AFAICS there's no downside.
However, I realized that the commit that added the function
(d435f15fff3c) did not make any such change at all -- it only changed
SysCacheGetAttr calls to use the new function, but no heap_getattr.
And we don't seem to have added such calls after.

Essentially the possibly contentious point is that the tuple we'd be
deforming did not come from syscache, but from a systable scan, so
calling a syscache function on it could be seen as breaking some API.
(Of course, this only works if there is a syscache on the given
relation.)

But we do have precedent: for example RelationGetFKeyList uses a sysscan
to feed DeconstructFkConstraintRow(), which extracts several attributes
that way using the CONSTROID syscache.

Does anybody think this could be a problem, if we extended it to be more
widely used?

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Los cuentos de hadas no dan al niño su primera idea sobre los monstruos.
Lo que le dan es su primera idea de la posible derrota del monstruo."
                                                   (G. K. Chesterton)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I was about to push a quick patch to replace the use of heap_getattr()
> in get_primary_key_attnos() with SysCacheGetAttrNotNull(), because that
> makes the code a few lines shorter and AFAICS there's no downside.
> However, I realized that the commit that added the function
> (d435f15fff3c) did not make any such change at all -- it only changed
> SysCacheGetAttr calls to use the new function, but no heap_getattr.
> And we don't seem to have added such calls after.

Seems to me it'd be more consistent to invent a wrapper function
heap_getattr_notnull() that adds the same sort of error check,
instead of abusing the syscache function as you suggest.  For one
thing, then the functionality could be used whether there's a
suitable syscache or not.

            regards, tom lane