Re: addRangeTableEntry() relies on pstate, contrary to its documentation

On 2015-01-04 12:33:34 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > Off list Tom commented that suggestion with:
> >> An easy alternative fix, of course, is to not call isLockedRefname if
> >> we don't have a pstate (or else put the pstate==NULL test inside it).
>
> > I'm not a big fan of that - won't that essentially cause the wrong
> > locklevel to be used and thus open the door for lock upgrade deadlocks?
>
> Well, it would amount to assuming that the table was not mentioned in
> "FOR UPDATE". Depending on context, that might be perfectly appropriate.

Yea. Given that there's apparently (given no reports of crashes in the
last couple years) not even indirect callers it's a bit hard to say
;). Given that it seems to be the easiest way to handle this, even
though it's not a nice fix.

> A quick grep finds these places that are visibly passing NULL to one or
> another addRangeTableEntry* function:
>
> convert_ANY_sublink_to_join(): pulls up an ANY subquery with
>
> rte = addRangeTableEntryForSubquery(NULL, ...
>
> UpdateRangeTableOfViewParse(): inserts NEW/OLD RTEs using
>
> rt_entry1 = addRangeTableEntryForRelation(NULL, viewRel,
> makeAlias("old", NIL),
> false, false);
> rt_entry2 = addRangeTableEntryForRelation(NULL, viewRel,
> makeAlias("new", NIL),
> false, false);

Yea, found those as well by now... There used to be a some more in the
past, but never many afaics.

> An alternative of course is to not have this API spec for all
> addRangeTableEntry* functions, but just the two used this way.
> I don't much care for that though.

Yea :(. And creating a faux pstate for the above callers isn't
particularly nice either.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services