Обсуждение: addRangeTableEntry() relies on pstate, contrary to its documentation

Hi,

Since at least 61e532820824504aa92ad93c427722d3fa9c1632 from 2009,
addRangeTableEntry() relies pstate being != NULL via its call to
isLockedRefname() even though its documentation says:* If pstate is NULL, we just build an RTE and return it without
addingit* to an rtable list.
 

I think we should just remove the above sentence and code supporting it
from addRangeTableEntry* and add asserts ensuring its passed in.

Off list Tom commented that suggestion with:
> NAK.  I'm absolutely certain that there is, or at least once was, code
> that relied on that feature.  Maybe not for addRangeTableEntry itself,
> but for at least one of its siblings.

Yea, there had to be, for the code to be written that way. I'm not
exactly an expert in that area of the code, and lots of it predates my
involvement in the project...

> Before removing the feature I'd
> want to see a trace-down of where that usage went away and an analysis
> of why the need for it won't come back.

Ok. I've only cursorily checked callers. The number of callchains to all
of them make it hard to verify it conclusively :(

> 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?

Greetings,

Andres Freund

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

Andres Freund <andres@2ndquadrant.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.

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);
 

So you would certainly break these callers.  I'm not sure whether any of
the callers that are passing down their own pstate arguments can ever be
passed a NULL; I'm inclined to doubt it though.

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.
        regards, tom lane

On 2015-01-04 12:33:34 -0500, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.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