Re: fastgetattr & isNull

On Wed, Jan 6, 2010 at 1:16 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Spoke with Bruce on IM and we think the best option is to just remove
>> the NULL tests.  Since it's been this way for 11 years, presumably
>> nobody is trying to use it with a NULL fourth argument.
>
>> Proposed patch attached.
>
> There are a number of is-null checks in related code that ought to go
> away too --- look at heap_getattr, nocachegetattr, etc.  Our principle
> here ought to be that none of the field-fetching routines allow a null
> pointer.

Revised patch attached. Blow-by-blow:

- fastgetattr() is both a macro and a function. I previously fixed
the macro; now I've made the function correspond.
- heap_getattr() is always a macro. The previous version ripped out
the single NULL test therein and this one does the same thing.
- nocachegetattr() already documents that it can't be called when the
attribute being fetched is null, but for some reason it still has an
isNull argument and a bunch of residual cruft, which I have ripped
out, for a substantial improvement in readability, IMHO.
- heap_getsysattr() has an if (isnull) test, which I have removed.
- index_getattr() already follows the proposed coding standard, so no change.
- nocache_index_getattr() is a lobotomized clone of nocachegetattr()
right down to the duplicated comment (gotta love that), and I've given
it the same treatment.
- slot_getattr() already follows the proposed coding standard, so no change.

The naming consistency of these functions and macros leaves a great
deal to be desired. The arrangement whereby we have a macro called
fetchatt() which calls a macro called fetch_att() is particularly
jaw-dropping.

...Robert