diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c index 7bf4e55c7a1..cc10c5c1ddc 100644 --- a/src/backend/optimizer/prep/prepjointree.c +++ b/src/backend/optimizer/prep/prepjointree.c @@ -158,13 +158,15 @@ static void reduce_outer_joins_pass2(Node *jtnode, static void report_reduced_full_join(reduce_outer_joins_pass2_state *state2, int rtindex, Relids relids); static Node *remove_useless_results_recurse(PlannerInfo *root, Node *jtnode, + Relids baserels, Node **parent_quals, Relids *dropped_outer_joins); static int get_result_relid(PlannerInfo *root, Node *jtnode); static void remove_result_refs(PlannerInfo *root, int varno, Node *newjtloc); -static bool find_dependent_phvs(PlannerInfo *root, int varno); +static bool find_dependent_phvs(PlannerInfo *root, int varno, Relids baserels); static bool find_dependent_phvs_in_jointree(PlannerInfo *root, - Node *node, int varno); + Node *node, int varno, + Relids baserels); static void substitute_phv_relids(Node *node, int varno, Relids subrelids); static void fix_append_rel_relids(PlannerInfo *root, int varno, @@ -3614,15 +3616,24 @@ report_reduced_full_join(reduce_outer_joins_pass2_state *state2, void remove_useless_result_rtes(PlannerInfo *root) { + Relids baserels; Relids dropped_outer_joins = NULL; ListCell *cell; + /* + * We'll need the set of baserels in the jointree to perform + * find_dependent_phvs() checks. + */ + baserels = get_relids_in_jointree((Node *) root->parse->jointree, + false, false); + /* Top level of jointree must always be a FromExpr */ Assert(IsA(root->parse->jointree, FromExpr)); /* Recurse ... */ root->parse->jointree = (FromExpr *) remove_useless_results_recurse(root, (Node *) root->parse->jointree, + baserels, NULL, &dropped_outer_joins); /* We should still have a FromExpr */ @@ -3683,9 +3694,12 @@ remove_useless_result_rtes(PlannerInfo *root) * the parent's quals list; otherwise, pass NULL for parent_quals. * (Note that in some cases, parent_quals points to the quals of a parent * more than one level up in the tree.) + * + * baserels is the set of base (non-join) RT indexes in the whole jointree. */ static Node * remove_useless_results_recurse(PlannerInfo *root, Node *jtnode, + Relids baserels, Node **parent_quals, Relids *dropped_outer_joins) { @@ -3716,6 +3730,7 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode, /* Recursively transform child, allowing it to push up quals ... */ child = remove_useless_results_recurse(root, child, + baserels, &f->quals, dropped_outer_joins); /* ... and stick it back into the tree */ @@ -3729,7 +3744,8 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode, */ if (list_length(f->fromlist) > 1 && (varno = get_result_relid(root, child)) != 0 && - !find_dependent_phvs_in_jointree(root, (Node *) f, varno)) + !find_dependent_phvs_in_jointree(root, (Node *) f, varno, + baserels)) { f->fromlist = foreach_delete_current(f->fromlist, cell); result_relids = bms_add_member(result_relids, varno); @@ -3798,12 +3814,14 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode, * quals up, or at least there's no particular reason to. */ j->larg = remove_useless_results_recurse(root, j->larg, + baserels, (j->jointype == JOIN_INNER) ? &j->quals : (j->jointype == JOIN_LEFT) ? parent_quals : NULL, dropped_outer_joins); j->rarg = remove_useless_results_recurse(root, j->rarg, + baserels, (j->jointype == JOIN_INNER || j->jointype == JOIN_LEFT) ? &j->quals : NULL, @@ -3831,7 +3849,8 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode, * allowed to have such refs. */ if ((varno = get_result_relid(root, j->larg)) != 0 && - !find_dependent_phvs_in_jointree(root, j->rarg, varno)) + !find_dependent_phvs_in_jointree(root, j->rarg, varno, + baserels)) { remove_result_refs(root, varno, j->rarg); if (j->quals != NULL && parent_quals == NULL) @@ -3886,7 +3905,7 @@ remove_useless_results_recurse(PlannerInfo *root, Node *jtnode, */ if ((varno = get_result_relid(root, j->rarg)) != 0 && (j->quals == NULL || - !find_dependent_phvs(root, varno))) + !find_dependent_phvs(root, varno, baserels))) { remove_result_refs(root, varno, j->larg); *dropped_outer_joins = bms_add_member(*dropped_outer_joins, @@ -4008,9 +4027,17 @@ remove_result_refs(PlannerInfo *root, int varno, Node *newjtloc) /* - * find_dependent_phvs - are there any PlaceHolderVars whose relids are + * find_dependent_phvs - are there any PlaceHolderVars whose base relids are * exactly the given varno? * + * We ignore outer-join relids present in a PHV's phrels, by intersecting + * with the caller-supplied "baserels" set. This is necessary in part + * because some of the OJ relids may be stale, that is we may have + * already decided to remove those joins in remove_useless_result_rtes + * and not yet have cleaned their relid bits out of upper PHVs. + * But in general, it's the set of baserels that identify possible places + * to evaluate a PHV, and we mustn't let that go to empty. + * * find_dependent_phvs should be used when we want to see if there are * any such PHVs anywhere in the Query. Another use-case is to see if * a subtree of the join tree contains such PHVs; but for that, we have @@ -4020,8 +4047,9 @@ remove_result_refs(PlannerInfo *root, int varno, Node *newjtloc) typedef struct { - Relids relids; - int sublevels_up; + Relids relids; /* target relid, represented as a relid set */ + Relids baserels; /* set of base (non-OJ) RT indexes in query */ + int sublevels_up; /* current nesting level */ } find_dependent_phvs_context; static bool @@ -4034,9 +4062,16 @@ find_dependent_phvs_walker(Node *node, { PlaceHolderVar *phv = (PlaceHolderVar *) node; - if (phv->phlevelsup == context->sublevels_up && - bms_equal(context->relids, phv->phrels)) - return true; + if (phv->phlevelsup == context->sublevels_up) + { + Relids phbaserels = bms_intersect(phv->phrels, + context->baserels); + bool match = bms_equal(context->relids, phbaserels); + + bms_free(phbaserels); + if (match) + return true; + } /* fall through to examine children */ } if (IsA(node, Query)) @@ -4060,7 +4095,7 @@ find_dependent_phvs_walker(Node *node, } static bool -find_dependent_phvs(PlannerInfo *root, int varno) +find_dependent_phvs(PlannerInfo *root, int varno, Relids baserels) { find_dependent_phvs_context context; @@ -4069,6 +4104,7 @@ find_dependent_phvs(PlannerInfo *root, int varno) return false; context.relids = bms_make_singleton(varno); + context.baserels = baserels; context.sublevels_up = 0; if (query_tree_walker(root->parse, find_dependent_phvs_walker, &context, 0)) @@ -4082,7 +4118,8 @@ find_dependent_phvs(PlannerInfo *root, int varno) } static bool -find_dependent_phvs_in_jointree(PlannerInfo *root, Node *node, int varno) +find_dependent_phvs_in_jointree(PlannerInfo *root, Node *node, int varno, + Relids baserels) { find_dependent_phvs_context context; Relids subrelids; @@ -4093,6 +4130,7 @@ find_dependent_phvs_in_jointree(PlannerInfo *root, Node *node, int varno) return false; context.relids = bms_make_singleton(varno); + context.baserels = baserels; context.sublevels_up = 0; /* diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 86078cbf27d..9fc12fb0cb0 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -4133,6 +4133,33 @@ select * from 1 | 2 | 2 (1 row) +-- Also, we mustn't remove an RTE_RESULT that is the only baserel where a PHV +-- can be evaluated, even when the PHV's phrels also mention an outer join. +explain (verbose, costs off) +select * from (values (1),(2)) v(x) + left join (select q from (select 7 as q from (select where false) ss1) ss2 + left join (select 8 as z) ss3 on true) ss4 on true; + QUERY PLAN +------------------------------------ + Nested Loop Left Join + Output: "*VALUES*".column1, (7) + Join Filter: false + -> Values Scan on "*VALUES*" + Output: "*VALUES*".column1 + -> Result + Output: 7 + One-Time Filter: false +(8 rows) + +select * from (values (1),(2)) v(x) + left join (select q from (select 7 as q from (select where false) ss1) ss2 + left join (select 8 as z) ss3 on true) ss4 on true; + x | q +---+--- + 1 | + 2 | +(2 rows) + -- This example demonstrates the folly of our old "have_dangerous_phv" logic begin; set local from_collapse_limit to 2; diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 3aaa882d668..478c8e59c95 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -1376,6 +1376,16 @@ select * from (select 1 as x) ss1 left join (select 2 as y) ss2 on (true), lateral (select ss2.y as z limit 1) ss3; +-- Also, we mustn't remove an RTE_RESULT that is the only baserel where a PHV +-- can be evaluated, even when the PHV's phrels also mention an outer join. +explain (verbose, costs off) +select * from (values (1),(2)) v(x) + left join (select q from (select 7 as q from (select where false) ss1) ss2 + left join (select 8 as z) ss3 on true) ss4 on true; +select * from (values (1),(2)) v(x) + left join (select q from (select 7 as q from (select where false) ss1) ss2 + left join (select 8 as z) ss3 on true) ss4 on true; + -- This example demonstrates the folly of our old "have_dangerous_phv" logic begin; set local from_collapse_limit to 2;