Re: fastgetattr & isNull

From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: fastgetattr & isNull
Date: 2010-01-06 19:34:02
Message-ID: 603c8f071001061134k4041d76bx85446927f1c102d0@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
getattr-null-tests-v2.patch text/x-patch 11.4 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2010-01-06 19:36:41 Re: I: TODO: Allow substring/replace() to get/set bit values
Previous Message Andrew Dunstan 2010-01-06 19:27:59 Re: Status of plperl inter-sp calling