Re: [sqlsmith] Failed to generate plan on lateral subqueries

On Wed, Dec 9, 2015 at 4:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> David Fetter <david@fetter.org> writes:
>> On Tue, Dec 08, 2015 at 12:13:41PM -0500, Tom Lane wrote:
>>> Hm.  At least in the first of these cases, the problem is that the
>>> code I committed yesterday doesn't account for indirect lateral
>>> dependencies.  That is, if S1 depends on S2 which depends on the
>>> inner side of an outer join, it now knows not to join S2 directly to
>>> the outer side of the outer join, but it doesn't realize that the
>>> same must apply to S1.
>>>
>>> Maybe we should redefine lateral_relids as the transitive closure of
>>> a rel's lateral dependencies?  Not sure.
>
>> That seems like it would fix the problem once and for good.  Is there
>> any reason to believe that the lateral dependencies could go in a
>> loop, i.e. is there a reason to put in checks for same in the
>> transitive closure construction?
>
> It should not be possible to have a loop, since rels can only have lateral
> references to rels that appeared syntactically before them.  But an Assert
> about it doesn't seem a bad thing.
>
> After working on this for awhile, I've found that indeed converting
> lateral_relids to a transitive closure makes things better.  But there was
> another bug (or two other bugs, depending on how you count) exposed by
> Andreas' examples.  I was right after all to suspect that the "hazardous
> PHV" condition needs to be accounted for by join_is_legal; as things
> stood, join_is_legal might allow a join for which every possible path
> would be rejected by check_hazardous_phv.  More, if we were insisting that
> a join PHV be calculated before we could pass it to some other relation,
> we didn't actually do anything to ensure that that join would get built.
> Given an appropriate set of conditions, the clauseless-join heuristics
> could decide that that join wasn't interesting and never build it, again
> possibly leading to planner failure.  So join_is_legal's cousins
> have_join_order_restriction and has_join_restriction also need to know
> about this issue.
>
> (This is a bit of a mess; I'm beginning to wonder if we shouldn't bite
> the bullet and teach the executor how to handle non-Var NestLoopParams,
> so that the planner doesn't have to work around that.  But I'd rather
> not back-patch such a change.)
>
> I also noticed that lateral_referencers should be a transitive closure
> as well.  I think that oversight doesn't lead to any observable bug,
> but it would lead to constructing index paths that cannot be used, so
> fixing it should save some planner cycles.
>
> So that leads to the attached patch, which I think is good as-is for
> the back branches.  In this state of the code, the LateralJoinInfo
> list is darn near useless: the only thing we use it for is detecting
> whether two relations have a direct (not transitive closure) lateral
> reference.  I'm strongly tempted to replace that logic by keeping a
> copy of the pre-closure lateral_relids sets, and then we could drop
> LateralJoinInfo altogether, which would make create_lateral_join_info
> quite a bit shorter and faster.  That could go into HEAD/9.5, but
> I'm afraid to back-patch it further than 9.5 for fear that third-party
> code somewhere might be relying on the LateralJoinInfo data structure.

Aside from the functional issues, could your changes result in
performance regressions? (if so, I'd argue not to backpatch unless
there were cases that returned bad data as opposed to spurious
errors).

merlin