From f50f029c497da35927491f16964f7aa993ecbae5 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 13 Feb 2023 11:45:32 -0500 Subject: [PATCH] Fix thinkos in have_unsafe_outer_join_ref; reduce to Assert check. Late in the development of commit 2489d76c4, I (tgl) incorrectly concluded that the new function have_unsafe_outer_join_ref couldn't ever reach its inner loop. That should be the case if the inner rel's parameterization is based on just one Var, but it could be based on Vars from several relations, and then not only is the inner loop reachable but it's wrongly coded. Despite those errors, it still appears that the whole thing is redundant given previous join_is_legal checks, so let's arrange to only run it in assert-enabled builds. Diagnosis and patch by Richard Guo, per fuzz testing by Justin Pryzby. Discussion: https://postgr.es/m/20230212235823.GW1653@telsasoft.com --- src/backend/optimizer/path/joinpath.c | 29 +++++++++++++-------------- src/test/regress/expected/join.out | 26 ++++++++++++++++++++++++ src/test/regress/sql/join.sql | 17 ++++++++++++++++ 3 files changed, 57 insertions(+), 15 deletions(-) diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c index 9d4a9197ee6ed..e6ef0deb23492 100644 --- a/src/backend/optimizer/path/joinpath.c +++ b/src/backend/optimizer/path/joinpath.c @@ -378,9 +378,9 @@ allow_star_schema_join(PlannerInfo *root, * restrictions prevent us from attempting a join that would cause a problem. * (That's unsurprising, because the code worked before we ever added * outer-join relids to expression relids.) It still seems worth checking - * as a backstop, but we don't go to a lot of trouble: just reject if the - * unsatisfied part includes any outer-join relids at all. + * as a backstop, but we only do so in assert-enabled builds. */ +#ifdef USE_ASSERT_CHECKING static inline bool have_unsafe_outer_join_ref(PlannerInfo *root, Relids outerrelids, @@ -388,11 +388,10 @@ have_unsafe_outer_join_ref(PlannerInfo *root, { bool result = false; Relids unsatisfied = bms_difference(inner_paramrels, outerrelids); + Relids satisfied = bms_intersect(inner_paramrels, outerrelids); - if (unlikely(bms_overlap(unsatisfied, root->outer_join_rels))) + if (bms_overlap(unsatisfied, root->outer_join_rels)) { -#ifdef NOT_USED - /* If we ever weaken the join order restrictions, we might need this */ ListCell *lc; foreach(lc, root->join_info_list) @@ -401,25 +400,23 @@ have_unsafe_outer_join_ref(PlannerInfo *root, if (!bms_is_member(sjinfo->ojrelid, unsatisfied)) continue; /* not relevant */ - if (bms_overlap(inner_paramrels, sjinfo->min_righthand) || + if (bms_overlap(satisfied, sjinfo->min_righthand) || (sjinfo->jointype == JOIN_FULL && - bms_overlap(inner_paramrels, sjinfo->min_lefthand))) + bms_overlap(satisfied, sjinfo->min_lefthand))) { result = true; /* doesn't work */ break; } } -#else - /* For now, if we do see an overlap, just assume it's trouble */ - result = true; -#endif } /* Waste no memory when we reject a path here */ bms_free(unsatisfied); + bms_free(satisfied); return result; } +#endif /* USE_ASSERT_CHECKING */ /* * paraminfo_get_equal_hashops @@ -713,16 +710,15 @@ try_nestloop_path(PlannerInfo *root, /* * Check to see if proposed path is still parameterized, and reject if the * parameterization wouldn't be sensible --- unless allow_star_schema_join - * says to allow it anyway. Also, we must reject if either - * have_unsafe_outer_join_ref or have_dangerous_phv don't like the look of - * it, which could only happen if the nestloop is still parameterized. + * says to allow it anyway. Also, we must reject if have_dangerous_phv + * doesn't like the look of it, which could only happen if the nestloop is + * still parameterized. */ required_outer = calc_nestloop_required_outer(outerrelids, outer_paramrels, innerrelids, inner_paramrels); if (required_outer && ((!bms_overlap(required_outer, extra->param_source_rels) && !allow_star_schema_join(root, outerrelids, inner_paramrels)) || - have_unsafe_outer_join_ref(root, outerrelids, inner_paramrels) || have_dangerous_phv(root, outerrelids, inner_paramrels))) { /* Waste no memory when we reject a path here */ @@ -730,6 +726,9 @@ try_nestloop_path(PlannerInfo *root, return; } + /* If we got past that, we shouldn't have any unsafe outer-join refs */ + Assert(!have_unsafe_outer_join_ref(root, outerrelids, inner_paramrels)); + /* * Do a precheck to quickly eliminate obviously-inferior paths. We * calculate a cheap lower bound on the path's cost and then use diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 2f1f8b8dbe673..db9512969f373 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -4653,6 +4653,32 @@ where tt1.f1 = ss1.c0; ---------- (0 rows) +explain (verbose, costs off) +select 1 from + int4_tbl as i4 + inner join + ((select 42 as n from int4_tbl x1 left join int8_tbl x2 on f1 = q1) as ss1 + right join (select 1 as z) as ss2 on true) + on false, + lateral (select i4.f1, ss1.n from int8_tbl as i8 limit 1) as ss3; + QUERY PLAN +-------------------------- + Result + Output: 1 + One-Time Filter: false +(3 rows) + +select 1 from + int4_tbl as i4 + inner join + ((select 42 as n from int4_tbl x1 left join int8_tbl x2 on f1 = q1) as ss1 + right join (select 1 as z) as ss2 on true) + on false, + lateral (select i4.f1, ss1.n from int8_tbl as i8 limit 1) as ss3; + ?column? +---------- +(0 rows) + -- -- check a case in which a PlaceHolderVar forces join order -- diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 400c16958f314..2ff68879d2287 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -1609,6 +1609,23 @@ select 1 from lateral (select tt4.f1 as c0 from text_tbl as tt5 limit 1) as ss1 where tt1.f1 = ss1.c0; +explain (verbose, costs off) +select 1 from + int4_tbl as i4 + inner join + ((select 42 as n from int4_tbl x1 left join int8_tbl x2 on f1 = q1) as ss1 + right join (select 1 as z) as ss2 on true) + on false, + lateral (select i4.f1, ss1.n from int8_tbl as i8 limit 1) as ss3; + +select 1 from + int4_tbl as i4 + inner join + ((select 42 as n from int4_tbl x1 left join int8_tbl x2 on f1 = q1) as ss1 + right join (select 1 as z) as ss2 on true) + on false, + lateral (select i4.f1, ss1.n from int8_tbl as i8 limit 1) as ss3; + -- -- check a case in which a PlaceHolderVar forces join order --