On Wed, Jan 6, 2010 at 1:16 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Robert Haas <robertmh...@gmail.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
*** a/src/backend/access/common/heaptuple.c
--- b/src/backend/access/common/heaptuple.c
***************
*** 323,330 **** heap_attisnull(HeapTuple tup, int attnum)
Datum
nocachegetattr(HeapTuple tuple,
int attnum,
! TupleDesc tupleDesc,
! bool *isnull)
{
HeapTupleHeader tup = tuple->t_data;
Form_pg_attribute *att = tupleDesc->attrs;
--- 323,329 ----
Datum
nocachegetattr(HeapTuple tuple,
int attnum,
! TupleDesc tupleDesc)
{
HeapTupleHeader tup = tuple->t_data;
Form_pg_attribute *att = tupleDesc->attrs;
***************
*** 333,340 **** nocachegetattr(HeapTuple tuple,
bool slow = false; /* do we have to walk attrs? */
int off; /* current offset within data */
- (void) isnull; /* not used */
-
/* ----------------
* Three cases:
*
--- 332,337 ----
***************
*** 344,411 **** nocachegetattr(HeapTuple tuple,
* ----------------
*/
- #ifdef IN_MACRO
- /* This is handled in the macro */
- Assert(attnum > 0);
-
- if (isnull)
- *isnull = false;
- #endif
-
attnum--;
! if (HeapTupleNoNulls(tuple))
! {
! #ifdef IN_MACRO
! /* This is handled in the macro */
! if (att[attnum]->attcacheoff >= 0)
! {
! return fetchatt(att[attnum],
! (char *) tup + tup->t_hoff +
! att[attnum]->attcacheoff);
! }
! #endif
! }
! else
{
/*
* there's a null somewhere in the tuple
*
! * check to see if desired att is null
*/
! #ifdef IN_MACRO
! /* This is handled in the macro */
! if (att_isnull(attnum, bp))
! {
! if (isnull)
! *isnull = true;
! return (Datum) NULL;
! }
! #endif
!
! /*
! * Now check to see if any preceding bits are null...
! */
{
! int byte = attnum >> 3;
! int finalbit = attnum & 0x07;
! /* check for nulls "before" final bit of last byte */
! if ((~bp[byte]) & ((1 << finalbit) - 1))
! slow = true;
! else
{
! /* check for nulls in any "earlier" bytes */
! int i;
!
! for (i = 0; i < byte; i++)
{
! if (bp[i] != 0xFF)
! {
! slow = true;
! break;
! }
}
}
}
--- 341,372 ----
* ----------------
*/
attnum--;
! if (!HeapTupleNoNulls(tuple))
{
/*
* there's a null somewhere in the tuple
*
! * check to see if any preceding bits are null...
*/
+ int byte = attnum >> 3;
+ int finalbit = attnum & 0x07;
! /* check for nulls "before" final bit of last byte */
! if ((~bp[byte]) & ((1 << finalbit) - 1))
! slow = true;
! else
{
! /* check for nulls in any "earlier" bytes */
! int i;
! for (i = 0; i < byte; i++)
{
! if (bp[i] != 0xFF)
{
! slow = true;
! break;
}
}
}
***************
*** 567,574 **** heap_getsysattr(HeapTuple tup, int attnum, TupleDesc tupleDesc, bool *isnull)
Assert(tup);
/* Currently, no sys attribute ever reads as NULL. */
! if (isnull)
! *isnull = false;
switch (attnum)
{
--- 528,534 ----
Assert(tup);
/* Currently, no sys attribute ever reads as NULL. */
! *isnull = false;
switch (attnum)
{
*** a/src/backend/access/common/indextuple.c
--- b/src/backend/access/common/indextuple.c
***************
*** 200,207 **** index_form_tuple(TupleDesc tupleDescriptor,
Datum
nocache_index_getattr(IndexTuple tup,
int attnum,
! TupleDesc tupleDesc,
! bool *isnull)
{
Form_pg_attribute *att = tupleDesc->attrs;
char *tp; /* ptr to data part of tuple */
--- 200,206 ----
Datum
nocache_index_getattr(IndexTuple tup,
int attnum,
! TupleDesc tupleDesc)
{
Form_pg_attribute *att = tupleDesc->attrs;
char *tp; /* ptr to data part of tuple */
***************
*** 210,217 **** nocache_index_getattr(IndexTuple tup,
int data_off; /* tuple data offset */
int off; /* current offset within data */
- (void) isnull; /* not used */
-
/* ----------------
* Three cases:
*
--- 209,214 ----
***************
*** 221,251 **** nocache_index_getattr(IndexTuple tup,
* ----------------
*/
- #ifdef IN_MACRO
- /* This is handled in the macro */
- Assert(PointerIsValid(isnull));
- Assert(attnum > 0);
-
- *isnull = false;
- #endif
-
data_off = IndexInfoFindDataOffset(tup->t_info);
attnum--;
! if (!IndexTupleHasNulls(tup))
! {
! #ifdef IN_MACRO
! /* This is handled in the macro */
! if (att[attnum]->attcacheoff >= 0)
! {
! return fetchatt(att[attnum],
! (char *) tup + data_off +
! att[attnum]->attcacheoff);
! }
! #endif
! }
! else
{
/*
* there's a null somewhere in the tuple
--- 218,228 ----
* ----------------
*/
data_off = IndexInfoFindDataOffset(tup->t_info);
attnum--;
! if (IndexTupleHasNulls(tup))
{
/*
* there's a null somewhere in the tuple
***************
*** 256,271 **** nocache_index_getattr(IndexTuple tup,
/* XXX "knows" t_bits are just after fixed tuple header! */
bp = (bits8 *) ((char *) tup + sizeof(IndexTupleData));
- #ifdef IN_MACRO
- /* This is handled in the macro */
-
- if (att_isnull(attnum, bp))
- {
- *isnull = true;
- return (Datum) NULL;
- }
- #endif
-
/*
* Now check to see if any preceding bits are null...
*/
--- 233,238 ----
*** a/src/backend/access/heap/heapam.c
--- b/src/backend/access/heap/heapam.c
***************
*** 834,840 **** fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
return (
(attnum) > 0 ?
(
! ((isnull) ? (*(isnull) = false) : (dummyret) NULL),
HeapTupleNoNulls(tup) ?
(
(tupleDesc)->attrs[(attnum) - 1]->attcacheoff >= 0 ?
--- 834,840 ----
return (
(attnum) > 0 ?
(
! (*(isnull) = false),
HeapTupleNoNulls(tup) ?
(
(tupleDesc)->attrs[(attnum) - 1]->attcacheoff >= 0 ?
***************
*** 844,861 **** fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
(tupleDesc)->attrs[(attnum) - 1]->attcacheoff)
)
:
! nocachegetattr((tup), (attnum), (tupleDesc), (isnull))
)
:
(
att_isnull((attnum) - 1, (tup)->t_data->t_bits) ?
(
! ((isnull) ? (*(isnull) = true) : (dummyret) NULL),
(Datum) NULL
)
:
(
! nocachegetattr((tup), (attnum), (tupleDesc), (isnull))
)
)
)
--- 844,861 ----
(tupleDesc)->attrs[(attnum) - 1]->attcacheoff)
)
:
! nocachegetattr((tup), (attnum), (tupleDesc))
)
:
(
att_isnull((attnum) - 1, (tup)->t_data->t_bits) ?
(
! (*(isnull) = true),
(Datum) NULL
)
:
(
! nocachegetattr((tup), (attnum), (tupleDesc))
)
)
)
*** a/src/include/access/htup.h
--- b/src/include/access/htup.h
***************
*** 763,769 **** extern void HeapTupleHeaderAdjustCmax(HeapTupleHeader tup,
#define fastgetattr(tup, attnum, tupleDesc, isnull) \
( \
AssertMacro((attnum) > 0), \
! (((isnull) != NULL) ? (*(isnull) = false) : (dummyret)NULL), \
HeapTupleNoNulls(tup) ? \
( \
(tupleDesc)->attrs[(attnum)-1]->attcacheoff >= 0 ? \
--- 763,769 ----
#define fastgetattr(tup, attnum, tupleDesc, isnull) \
( \
AssertMacro((attnum) > 0), \
! (*(isnull) = false), \
HeapTupleNoNulls(tup) ? \
( \
(tupleDesc)->attrs[(attnum)-1]->attcacheoff >= 0 ? \
***************
*** 773,790 **** extern void HeapTupleHeaderAdjustCmax(HeapTupleHeader tup,
(tupleDesc)->attrs[(attnum)-1]->attcacheoff) \
) \
: \
! nocachegetattr((tup), (attnum), (tupleDesc), (isnull)) \
) \
: \
( \
att_isnull((attnum)-1, (tup)->t_data->t_bits) ? \
( \
! (((isnull) != NULL) ? (*(isnull) = true) : (dummyret)NULL), \
(Datum)NULL \
) \
: \
( \
! nocachegetattr((tup), (attnum), (tupleDesc), (isnull)) \
) \
) \
)
--- 773,790 ----
(tupleDesc)->attrs[(attnum)-1]->attcacheoff) \
) \
: \
! nocachegetattr((tup), (attnum), (tupleDesc)) \
) \
: \
( \
att_isnull((attnum)-1, (tup)->t_data->t_bits) ? \
( \
! (*(isnull) = true), \
(Datum)NULL \
) \
: \
( \
! nocachegetattr((tup), (attnum), (tupleDesc)) \
) \
) \
)
***************
*** 818,824 **** extern Datum fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
( \
((attnum) > (int) HeapTupleHeaderGetNatts((tup)->t_data)) ? \
( \
! (((isnull) != NULL) ? (*(isnull) = true) : (dummyret)NULL), \
(Datum)NULL \
) \
: \
--- 818,824 ----
( \
((attnum) > (int) HeapTupleHeaderGetNatts((tup)->t_data)) ? \
( \
! (*(isnull) = true), \
(Datum)NULL \
) \
: \
***************
*** 838,844 **** extern void heap_fill_tuple(TupleDesc tupleDesc,
uint16 *infomask, bits8 *bit);
extern bool heap_attisnull(HeapTuple tup, int attnum);
extern Datum nocachegetattr(HeapTuple tup, int attnum,
! TupleDesc att, bool *isnull);
extern Datum heap_getsysattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
bool *isnull);
extern HeapTuple heap_copytuple(HeapTuple tuple);
--- 838,844 ----
uint16 *infomask, bits8 *bit);
extern bool heap_attisnull(HeapTuple tup, int attnum);
extern Datum nocachegetattr(HeapTuple tup, int attnum,
! TupleDesc att);
extern Datum heap_getsysattr(HeapTuple tup, int attnum, TupleDesc tupleDesc,
bool *isnull);
extern HeapTuple heap_copytuple(HeapTuple tuple);
*** a/src/include/access/itup.h
--- b/src/include/access/itup.h
***************
*** 110,116 **** typedef IndexAttributeBitMapData *IndexAttributeBitMap;
+ (tupleDesc)->attrs[(attnum)-1]->attcacheoff) \
) \
: \
! nocache_index_getattr((tup), (attnum), (tupleDesc), (isnull)) \
) \
: \
( \
--- 110,116 ----
+ (tupleDesc)->attrs[(attnum)-1]->attcacheoff) \
) \
: \
! nocache_index_getattr((tup), (attnum), (tupleDesc)) \
) \
: \
( \
***************
*** 121,127 **** typedef IndexAttributeBitMapData *IndexAttributeBitMap;
) \
: \
( \
! nocache_index_getattr((tup), (attnum), (tupleDesc), (isnull)) \
) \
) \
)
--- 121,127 ----
) \
: \
( \
! nocache_index_getattr((tup), (attnum), (tupleDesc)) \
) \
) \
)
***************
*** 142,148 **** typedef IndexAttributeBitMapData *IndexAttributeBitMap;
extern IndexTuple index_form_tuple(TupleDesc tupleDescriptor,
Datum *values, bool *isnull);
extern Datum nocache_index_getattr(IndexTuple tup, int attnum,
! TupleDesc tupleDesc, bool *isnull);
extern void index_deform_tuple(IndexTuple tup, TupleDesc tupleDescriptor,
Datum *values, bool *isnull);
extern IndexTuple CopyIndexTuple(IndexTuple source);
--- 142,148 ----
extern IndexTuple index_form_tuple(TupleDesc tupleDescriptor,
Datum *values, bool *isnull);
extern Datum nocache_index_getattr(IndexTuple tup, int attnum,
! TupleDesc tupleDesc);
extern void index_deform_tuple(IndexTuple tup, TupleDesc tupleDescriptor,
Datum *values, bool *isnull);
extern IndexTuple CopyIndexTuple(IndexTuple source);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
