On Wed, Jan 6, 2010 at 9:43 AM, Robert Haas <robertmh...@gmail.com> wrote: > The fastgetattr() attempts to make provision for the case where isnull > is a NULL pointer, but it doesn't seem to work. I tried it and got: > > relcache.c:494: error: invalid use of void expression > relcache.c:494: error: invalid use of void expression > relcache.c:494: warning: left-hand operand of comma expression has no effect > relcache.c:494: warning: left-hand operand of comma expression has no effect > > Changing the fourth argument from NULL to &isnull made the problem go away. > > I think we should either fix this so it actually works (if that's even > possible), or rip out the code that tries to cope with it. That > probably wouldn't produce any measurable speedup, but at least it > might save someone else some head-scratching the next time they're > trying to learn this code.
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. ...Robert
diff --git a/src/include/access/htup.h b/src/include/access/htup.h index a42782e..725b220 100644 --- a/src/include/access/htup.h +++ b/src/include/access/htup.h @@ -755,6 +755,8 @@ extern void HeapTupleHeaderAdjustCmax(HeapTupleHeader tup, * * This gets called many times, so we macro the cacheable and NULL * lookups, and call nocachegetattr() for the rest. + * + * isnull must not be a NULL pointer, as we don't test for that. * ---------------- */ @@ -763,7 +765,7 @@ extern void HeapTupleHeaderAdjustCmax(HeapTupleHeader tup, #define fastgetattr(tup, attnum, tupleDesc, isnull) \ ( \ AssertMacro((attnum) > 0), \ - (((isnull) != NULL) ? (*(isnull) = false) : (dummyret)NULL), \ + (*(isnull) = false), \ HeapTupleNoNulls(tup) ? \ ( \ (tupleDesc)->attrs[(attnum)-1]->attcacheoff >= 0 ? \ @@ -779,7 +781,7 @@ extern void HeapTupleHeaderAdjustCmax(HeapTupleHeader tup, ( \ att_isnull((attnum)-1, (tup)->t_data->t_bits) ? \ ( \ - (((isnull) != NULL) ? (*(isnull) = true) : (dummyret)NULL), \ + (*(isnull) = true), \ (Datum)NULL \ ) \ : \ @@ -804,6 +806,7 @@ extern Datum fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc, * * If the field in question has a NULL value, we return a zero Datum * and set *isnull == true. Otherwise, we set *isnull == false. + * isnull must not be a NULL pointer, as we don't test for that. * * <tup> is the pointer to the heap tuple. <attnum> is the attribute * number of the column (field) caller wants. <tupleDesc> is a @@ -818,7 +821,7 @@ extern Datum fastgetattr(HeapTuple tup, int attnum, TupleDesc tupleDesc, ( \ ((attnum) > (int) HeapTupleHeaderGetNatts((tup)->t_data)) ? \ ( \ - (((isnull) != NULL) ? (*(isnull) = true) : (dummyret)NULL), \ + (*(isnull) = true), \ (Datum)NULL \ ) \ : \
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
