Fix edge case in remove_useless_result_rtes() with outer joins.

find_dependent_phvs() and find_dependent_phvs_in_jointree() decide
whether a PlaceHolderVar depends on the RTE_RESULT rel we're
considering removing by comparing the PHV's phrels to a singleton set
containing that rel's RT index, reasoning that if phrels contains any
other relid bits then those define an appropriate place where we can
evaluate the PHV.  But since this code was originally written, we've
redefined phrels to include outer-join relids, and that breaks this
logic, potentially allowing us to remove an RTE_RESULT that leaves no
valid place to evaluate the PHV.  The planner doesn't throw an error
when that happens, but it does produce an incorrect plan that will not
replace the PHV's value with NULL when needed.

In the known test case for this bug, the "extra" OJ relid is one that
we've actually decided to remove but haven't yet cleaned out of the
query's PHVs.  It's not entirely clear though that that would always
be the case.  Let's restore this code to the way it was designed to
work, by considering only base relids within the PHV's phrels.

Bug: #19553
Reported-by: Viktor Leis <leis@in.tum.de>
Author: Matheus Alcantara <matheusssilv97@gmail.com>
Co-authored-by: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19553-4561747f93f368a7@postgresql.org
Backpatch-through: 16

diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 4424fdbe906b607ac0afa7346f5adfe21c4fcab5..2b7c9a1713644588b7da481d092b87c9bd1bc0db 100644 (file)
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -166,13 +166,15 @@ static void report_reduced_full_join(reduce_outer_joins_pass2_state *state2,
 static bool has_notnull_forced_var(PlannerInfo *root, List *forced_null_vars,
                                                                   reduce_outer_joins_pass1_state *right_state);
 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,
@@ -3886,15 +3888,24 @@ has_notnull_forced_var(PlannerInfo *root, List *forced_null_vars,
 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 */
@@ -3955,9 +3966,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)
 {
@@ -3988,6 +4002,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 */
@@ -4001,7 +4016,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);
@@ -4070,12 +4086,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,
@@ -4103,7 +4121,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)
@@ -4158,7 +4177,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,
@@ -4280,9 +4299,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
@@ -4292,8 +4319,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
@@ -4306,9 +4334,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))
@@ -4332,7 +4367,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;
 
@@ -4341,6 +4376,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))
@@ -4354,7 +4390,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;
@@ -4365,6 +4402,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 83bd5649d5c04504aa41d45fd48eb5eb83e63e59..19e2cca548b68b97f3826ee6eb914f6fedffbea5 100644 (file)
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -4228,6 +4228,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 32d4a5a677ea3c9179af12e6fb28eb693555bba4..85aed7bf70470c89f8c58c31ce921a34a6035166 100644 (file)
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -1422,6 +1422,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;