Re: Another bug(?) turned up by the llvm optimization checker

Greg Stark <stark@mit.edu> writes:
> The commit below, specifically the change mentioned in the last paragraph
> to fix isLockedRel broke the following comment in addRangeTableEntry:

>  * If pstate is NULL, we just build an RTE and return it without adding it
>  * to an rtable list.

> In fact isLockedRefname() will seg fault promptly if pstate is NULL. I'm
> not clear why this behaviour is needed though since as far as I can tell
> nowhere in the code calls addRangeTableEntry or any of its derivatives with
> pstate==NULL. I'm inclined to just remove the comment and the test for
> pstate==NULL lower down but I don't really know what the motivation for
> this promised behaviour was in the first place so I'm hesitant to do it on
> my own.

Hm.  I think you are right that this is dead code at the moment, but if
you look around you'll find that there are several places that call
addRangeTableEntry's sister routines with NULL pstate, eg look at the
addRangeTableEntryForRelation calls in view.c.  There may once have been
code that called plain addRangeTableEntry that way, or maybe not, but
I'm inclined to think we ought to keep the API contracts similar for
all those functions.

However, that logic doesn't immediately say whether it's better to
make these functions safe against null pstate arguments, or to insist
the callers conjure up a pstate.  After a quick look at the number
of pstate uses that have evolved in the addRangeTableEntryForFoo
functions, I'm inclined to think the latter might be the safer
course of action.  It's not that hard to make a dummy pstate,
and we could delete the logic for null pstate argument in multiple
places.  And not worry about whether the need to defend against
null pstate will propagate into called functions.

In any case, the issue looks bigger than just addRangeTableEntry
itself.  Do you want to write up a patch?
        regards, tom lane