On Mon, Apr 15, 2024 at 12:44 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Richard Guo <guofenglinux@gmail.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?
From:
PG Bug reporting form Date: Subject:
BUG #18437: The index scan result is more than the full scan result, and the primary key index has duplicate val
Есть вопросы? Напишите нам!
Соглашаюсь с условиями обработки персональных данных
✖
By continuing to browse this website, you agree to the use of cookies. Go to Privacy Policy.
