Обсуждение: NULL-pointer check and incorrect comment for pstate in addRangeTableEntry

Hi all,

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?

Regards,
--
Michael

On Wed, Feb 25, 2015 at 7:27 AM, Michael Paquier
<michael.paquier@gmail.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

On Wed, Mar 4, 2015 at 6:43 AM, Robert Haas wrote:
> That seems to make sense to me.  Committed.

Thanks.
-- 
Michael

<p dir="ltr">This sounded familiar... I pointed out the same thing a while back and Tom had some feedback on what to do
aboutit:<p dir="ltr"><a
href="http://www.postgresql.org/message-id/23294.1384142407@sss.pgh.pa.us">http://www.postgresql.org/message-id/23294.1384142407@sss.pgh.pa.us</a><div
class="gmail_quote">On4 Mar 2015 00:37, "Michael Paquier" <<a
href="mailto:michael.paquier@gmail.com">michael.paquier@gmail.com</a>>wrote:<br type="attribution" /><blockquote
class="gmail_quote"style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Wed, Mar 4, 2015 at 6:43
AM,Robert Haas wrote:<br /> > That seems to make sense to me.  Committed.<br /><br /> Thanks.<br /> --<br />
Michael<br/><br /><br /> --<br /> Sent via pgsql-hackers mailing list (<a
href="mailto:pgsql-hackers@postgresql.org">pgsql-hackers@postgresql.org</a>)<br/> To make changes to your
subscription:<br/><a href="http://www.postgresql.org/mailpref/pgsql-hackers"
target="_blank">http://www.postgresql.org/mailpref/pgsql-hackers</a><br/></blockquote></div> 

On Wed, Mar 4, 2015 at 9:12 PM, Greg Stark <stark@mit.edu> wrote:
> This sounded familiar... I pointed out the same thing a while back and Tom
> had some feedback on what to do about it:
>
> http://www.postgresql.org/message-id/23294.1384142407@sss.pgh.pa.us

Translated into code I guess that this gives the attached patch.
--
Michael

On Wed, Mar 4, 2015 at 12:38 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Translated into code I guess that this gives the attached patch.

Probably want to update a comment somewhere but yes, those are the
same three call sites I had found back then. I don't see any patch
lying around so I don't think I got any further than searching for
call sites.

-- 
greg

On Wed, Mar 4, 2015 at 7:38 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Mar 4, 2015 at 9:12 PM, Greg Stark <stark@mit.edu> wrote:
>> This sounded familiar... I pointed out the same thing a while back and Tom
>> had some feedback on what to do about it:
>>
>> http://www.postgresql.org/message-id/23294.1384142407@sss.pgh.pa.us
>
> Translated into code I guess that this gives the attached patch.

Looks fine to me, so committed.

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

On Thu, Mar 12, 2015 at 4:36 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Mar 4, 2015 at 7:38 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Wed, Mar 4, 2015 at 9:12 PM, Greg Stark <stark@mit.edu> wrote:
>>> This sounded familiar... I pointed out the same thing a while back and Tom
>>> had some feedback on what to do about it:
>>>
>>> http://www.postgresql.org/message-id/23294.1384142407@sss.pgh.pa.us
>>
>> Translated into code I guess that this gives the attached patch.
>
> Looks fine to me, so committed.

Thanks for picking this up.
-- 
Michael