Обсуждение: fastgetattr & isNull
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. ...Robert
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