NULL-pointer check and incorrect comment for pstate in addRangeTableEntry - Mailing list pgsql-hackers

From Michael Paquier
Subject NULL-pointer check and incorrect comment for pstate in addRangeTableEntry
Date
Msg-id CAB7nPqQ+Zz_ZLQEH9Se0K82ZfsmwyO5aP+D9fi3yupJ-Of_D6w@mail.gmail.com
Whole thread Raw
Responses Re: NULL-pointer check and incorrect comment for pstate in addRangeTableEntry  (Robert Haas <robertmhaas@gmail.com>)
List pgsql-hackers
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

Attachment

pgsql-hackers by date:

Previous
From: Andres Freund
Date:
Subject: Re: INSERT ... ON CONFLICT UPDATE and logical decoding
Next
From: Alvaro Herrera
Date:
Subject: Re: Add more tests on event triggers