From 89db83922a7f8ba223e233e262004b1745ece75d Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 7 Aug 2015 14:13:38 -0400 Subject: [PATCH] Further adjustments to PlaceHolderVar removal. A new test case from Andreas Seltenreich showed that we were still a bit confused about removing PlaceHolderVars during join removal. Specifically, remove_rel_from_query would remove a PHV that was used only underneath the removable join, even if the place where it's used was the join partner relation and not the join clause being deleted. This would lead to a "too late to create a new PlaceHolderInfo" error later on. We can defend against that by checking ph_eval_at to see if the PHV could possibly be getting used at some partner rel. Also improve some nearby LATERAL-related logic. I decided that the check on ph_lateral needed to take precedence over the check on ph_needed, in case there's a lateral reference underneath the join being considered. (That may be impossible, but I'm not convinced of it, and it's easy enough to defend against the case.) Also, I realized that remove_rel_from_query's logic for updating LateralJoinInfos is dead code, because we don't build those at all until after join removal. Back-patch to 9.3. Previous versions didn't have the LATERAL issues, of course, and they also didn't attempt to remove PlaceHolderInfos during join removal. (I'm starting to wonder if changing that was really such a great idea.) --- src/backend/optimizer/plan/analyzejoins.c | 44 +++++++++-------------- src/test/regress/expected/join.out | 41 +++++++++++++++++++++ src/test/regress/sql/join.sql | 25 +++++++++++++ 3 files changed, 83 insertions(+), 27 deletions(-) diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c index dfb2e37831..7912b153c5 100644 --- a/src/backend/optimizer/plan/analyzejoins.c +++ b/src/backend/optimizer/plan/analyzejoins.c @@ -240,10 +240,10 @@ join_is_removable(PlannerInfo *root, SpecialJoinInfo *sjinfo) { PlaceHolderInfo *phinfo = (PlaceHolderInfo *) lfirst(l); - if (bms_is_subset(phinfo->ph_needed, joinrelids)) - continue; /* PHV is not used above the join */ if (bms_overlap(phinfo->ph_lateral, innerrel->relids)) return false; /* it references innerrel laterally */ + if (bms_is_subset(phinfo->ph_needed, joinrelids)) + continue; /* PHV is not used above the join */ if (!bms_overlap(phinfo->ph_eval_at, innerrel->relids)) continue; /* it definitely doesn't reference innerrel */ if (bms_is_subset(phinfo->ph_eval_at, innerrel->relids)) @@ -439,47 +439,37 @@ remove_rel_from_query(PlannerInfo *root, int relid, Relids joinrelids) sjinfo->syn_righthand = bms_del_member(sjinfo->syn_righthand, relid); } - /* - * Likewise remove references from LateralJoinInfo data structures. - * - * If we are deleting a LATERAL subquery, we can forget its - * LateralJoinInfos altogether. Otherwise, make sure the target is not - * included in any lateral_lhs set. (It probably can't be, since that - * should have precluded deciding to remove it; but let's cope anyway.) - */ - for (l = list_head(root->lateral_info_list); l != NULL; l = nextl) - { - LateralJoinInfo *ljinfo = (LateralJoinInfo *) lfirst(l); - - nextl = lnext(l); - ljinfo->lateral_rhs = bms_del_member(ljinfo->lateral_rhs, relid); - if (bms_is_empty(ljinfo->lateral_rhs)) - root->lateral_info_list = list_delete_ptr(root->lateral_info_list, - ljinfo); - else - { - ljinfo->lateral_lhs = bms_del_member(ljinfo->lateral_lhs, relid); - Assert(!bms_is_empty(ljinfo->lateral_lhs)); - } - } + /* There shouldn't be any LATERAL info to translate, as yet */ + Assert(root->lateral_info_list == NIL); /* * Likewise remove references from PlaceHolderVar data structures, * removing any no-longer-needed placeholders entirely. + * + * Removal is a bit tricker than it might seem: we can remove PHVs that + * are used at the target rel and/or in the join qual, but not those that + * are used at join partner rels or above the join. It's not that easy to + * distinguish PHVs used at partner rels from those used in the join qual, + * since they will both have ph_needed sets that are subsets of + * joinrelids. However, a PHV used at a partner rel could not have the + * target rel in ph_eval_at, so we check that while deciding whether to + * remove or just update the PHV. There is no corresponding test in + * join_is_removable because it doesn't need to distinguish those cases. */ for (l = list_head(root->placeholder_list); l != NULL; l = nextl) { PlaceHolderInfo *phinfo = (PlaceHolderInfo *) lfirst(l); nextl = lnext(l); - if (bms_is_subset(phinfo->ph_needed, joinrelids)) + Assert(!bms_is_member(relid, phinfo->ph_lateral)); + if (bms_is_subset(phinfo->ph_needed, joinrelids) && + bms_is_member(relid, phinfo->ph_eval_at)) root->placeholder_list = list_delete_ptr(root->placeholder_list, phinfo); else { phinfo->ph_eval_at = bms_del_member(phinfo->ph_eval_at, relid); Assert(!bms_is_empty(phinfo->ph_eval_at)); - Assert(!bms_is_member(relid, phinfo->ph_lateral)); phinfo->ph_needed = bms_del_member(phinfo->ph_needed, relid); } } diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index d08f7052c6..da407efd09 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -3878,6 +3878,47 @@ select t1.* from Seq Scan on uniquetbl t1 (1 row) +explain (costs off) +select t0.* +from + text_tbl t0 + left join + (select case t1.ten when 0 then 'doh!'::text else null::text end as case1, + t1.stringu2 + from tenk1 t1 + join int4_tbl i4 ON i4.f1 = t1.unique2 + left join uniquetbl u1 ON u1.f1 = t1.string4) ss + on t0.f1 = ss.case1 +where ss.stringu2 !~* ss.case1; + QUERY PLAN +-------------------------------------------------------------------------------------------- + Nested Loop + Join Filter: (CASE t1.ten WHEN 0 THEN 'doh!'::text ELSE NULL::text END = t0.f1) + -> Nested Loop + -> Seq Scan on int4_tbl i4 + -> Index Scan using tenk1_unique2 on tenk1 t1 + Index Cond: (unique2 = i4.f1) + Filter: (stringu2 !~* CASE ten WHEN 0 THEN 'doh!'::text ELSE NULL::text END) + -> Materialize + -> Seq Scan on text_tbl t0 +(9 rows) + +select t0.* +from + text_tbl t0 + left join + (select case t1.ten when 0 then 'doh!'::text else null::text end as case1, + t1.stringu2 + from tenk1 t1 + join int4_tbl i4 ON i4.f1 = t1.unique2 + left join uniquetbl u1 ON u1.f1 = t1.string4) ss + on t0.f1 = ss.case1 +where ss.stringu2 !~* ss.case1; + f1 +------ + doh! +(1 row) + rollback; -- bug #8444: we've historically allowed duplicate aliases within aliased JOINs select * from diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 43ca7f16c2..f924532e8c 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -1260,6 +1260,31 @@ select t1.* from left join uniquetbl t3 on t2.d1 = t3.f1; +explain (costs off) +select t0.* +from + text_tbl t0 + left join + (select case t1.ten when 0 then 'doh!'::text else null::text end as case1, + t1.stringu2 + from tenk1 t1 + join int4_tbl i4 ON i4.f1 = t1.unique2 + left join uniquetbl u1 ON u1.f1 = t1.string4) ss + on t0.f1 = ss.case1 +where ss.stringu2 !~* ss.case1; + +select t0.* +from + text_tbl t0 + left join + (select case t1.ten when 0 then 'doh!'::text else null::text end as case1, + t1.stringu2 + from tenk1 t1 + join int4_tbl i4 ON i4.f1 = t1.unique2 + left join uniquetbl u1 ON u1.f1 = t1.string4) ss + on t0.f1 = ss.case1 +where ss.stringu2 !~* ss.case1; + rollback; -- bug #8444: we've historically allowed duplicate aliases within aliased JOINs -- 2.40.0