fastgetattr & isNull

On Wed, Jan 6, 2010 at 9:43 AM, Robert Haas <robertmhaas@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

Robert Haas <robertmhaas@gmail.com> writes:

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

Hmm. I think the macro means to handle the case where the argument is a
pointer variable whose value is null, not the case of writing "NULL" as
a literal argument.

Still, it's not entirely clear to me why ignoring the possibility of
a null value would be a good idea. So far as I can see, we have at
least the following coding pattern everywhere this is used:

fastgetattr(..., &isnull);
Assert(!isnull);

and I don't think it's good coding style to go without even an Assert.
So +1 for removing the support for a null pointer ...

regards, tom lane

Robert Haas escribi�:

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.

+1 for removing it. I used that macro as a pattern to write some other
macro to which I tried to pass a NULL value and wasted at least a couple
of hours trying to figure out why it wasn't working.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Robert Haas <robertmhaas@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.

I wouldn't bother with those added comments. They wouldn't have been
there if the code had always been like this. If you feel a need to
have a comment, it should be more like "Before Postgres 8.5, the isnull
argument could be a null pointer, but we no longer allow that". That
way tells people that there was a change here that might affect their
code, whereas the addition you suggest wouldn't flag that.

regards, tom lane

On Wed, Jan 6, 2010 at 1:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

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

Hmm.  I think the macro means to handle the case where the argument is a
pointer variable whose value is null, not the case of writing "NULL" as
a literal argument.

Hmm, I didn't think of that. I don't see any attempt at doing that in
the source code anywhere, though.

Still, it's not entirely clear to me why ignoring the possibility of
a null value would be a good idea.

It's harmless if (Datum) 0 can't be a datum of the relevant type,
because you can still distinguish whether you got a result back. But
you can always pass a dummy boolean point if you really want to do
that.

...Robert

On Wed, Jan 6, 2010 at 1:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@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.

I was just noticing this in the non-macro version of fastgetattr().
Let me go take a look at that.

I wouldn't bother with those added comments.  They wouldn't have been
there if the code had always been like this.  If you feel a need to
have a comment, it should be more like "Before Postgres 8.5, the isnull
argument could be a null pointer, but we no longer allow that".  That
way tells people that there was a change here that might affect their
code, whereas the addition you suggest wouldn't flag that.

Well, that comment is a bit misleading too, since a pointer with a
NULL value might work but a literal NULL certainly doesn't.

...Robert

Robert Haas <robertmhaas@gmail.com> writes:

Well, that comment is a bit misleading too, since a pointer with a
NULL value might work but a literal NULL certainly doesn't.

I think "(bool *) NULL" would work. What your compiler is complaining
about is trying to dereference a "void *" expression.

In practice, the people we'd need to reach with a comment would be ones
who had working code before --- and working code, in this context, would
most likely be code that was passing a pointer variable that contained
null. But as I said, I don't think it really requires any comment.

regards, tom lane

On Wed, Jan 6, 2010 at 1:38 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Well, that comment is a bit misleading too, since a pointer with a
NULL value might work but a literal NULL certainly doesn't.

I think "(bool *) NULL" would work.  What your compiler is complaining
about is trying to dereference a "void *" expression.

In practice, the people we'd need to reach with a comment would be ones
who had working code before --- and working code, in this context, would
most likely be code that was passing a pointer variable that contained
null.  But as I said, I don't think it really requires any comment.

I was less thinking of people whose code might break and more thinking
of people who might be trying to understand the preconditions for
using the macro. But on further reflection I think a lot more
documentation would be needed to really make that clear, so I'll skip
it for now.

...Robert

On Wed, Jan 6, 2010 at 1:16 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@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