Re: NULL-pointer check and incorrect comment for pstate in addRangeTableEntry

On Wed, Feb 25, 2015 at 7:27 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> Coverity is pointing out that addRangeTableEntry contains the
> following code path that does a NULL-pointer check on pstate:
> if (pstate != NULL)
> pstate->p_rtable = lappend(pstate->p_rtable, rte);
> But pstate is dereferenced before in isLockedRefname when grabbing the
> lock mode:
> lockmode = isLockedRefname(pstate, refname) ? RowShareLock : AccessShareLock;
>
> Note that there is as well the following comment that is confusing on
> top of addRangeTableEntry:
> * If pstate is NULL, we just build an RTE and return it without adding it
> * to an rtable list.
>
> So I have looked at all the code paths calling this function and found
> first that the only potential point where pstate could be NULL is
> transformTopLevelStmt in analyze.c. One level higher there are
> parse_analyze_varparams and parse_analyze that may use pstate as NULL,
> and even one level more higher in the stack there is
> pg_analyze_and_rewrite. But well, looking at each case individually,
> in all cases we never pass NULL for the parse tree node, so I think
> that we should remove the comment on top of addRangeTableEntry, remove
> the NULL-pointer check and add an assertion as in the patch attached.
>
> Thoughts?

That seems to make sense to me. Committed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company