Re: BUG #18429: Inconsistent results on similar queries with join lateral

On Mon, Apr 15, 2024 at 12:44 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> > Now that we've learned that join_clause_is_movable_into's heuristic
> > about physically referencing the target rel can fail for EC-derived
> > clauses, I'm kind of concerned that we may end up with duplicate clauses
> > in the final plan, since we do not check EC-derived clauses against
> > join_clause_is_movable_into in get_baserel_parampathinfo while we do in
> > create_nestloop_path. What if we have an EC-derived clause that in
> > get_baserel_parampathinfo it is put into ppi_clauses while in
> > create_nestloop_path it does not pass the movability checking? Is it
> > possible to occur, or is it just my illusion?
>
> I'm not sure either, but it certainly seems like a hazard. Also,
> get_joinrel_parampathinfo is really depending on getting consistent
> results from join_clause_is_movable_into to assign clauses to the
> correct join level. So after sleeping on it I think that "the
> results of generate_join_implied_equalities should pass
> join_clause_is_movable_into" is an invariant we don't really want to
> let go of, meaning that it would be a good idea to fix equivclass.c
> to ensure that's true in these child-rel corner cases. That's not
> very hard, requiring just a small hack in create_join_clause, as
> attached. It's not that much of a hack either because there are
> other places in equivclass.c that force the relid sets for child
> expressions to be more than what pull_varnos would conclude (search
> for comments mentioning pull_varnos).
>
> Having done that, we can add code in HEAD/v16 to assert that
> join_clause_is_movable_into is true here, while in the older branches
> we can use it to filter rather than klugily checking nullable_relids
> directly. So I end with the attached two patches.

+1 to both patches.

> I didn't include the new test case in the HEAD/v16 patch; since it
> doesn't represent a live bug for those branches I felt like maybe
> it's not worth the test cycles going forward. But there's certainly
> room for the opposite opinion. What do you think?

I agree that the new test case for v15 does not seem to be worth
including in v16+. It seems to me that it would be better if we can
have another new test case to verify that we've included child rel's
em_relids even for appendrel child relations with pseudoconstant
translated variables, i.e. to verify that the change in equivclass.c
takes effect. Maybe with a query like below:

explain (costs off)
select * from tenk1 t1
left join lateral
(select t1.unique1 as t1u, 0 as c
union all
select t1.unique1 as t1u, 1 as c) s on true
where t1.unique1 = s.c;

Without the change in equivclass.c, this query would trigger the new
added assert in get_baserel_parampathinfo for v16, and give a wrong plan
for v15. What do you think?

Thanks
Richard