Re: Ignored join clause

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> I think I was wrong, and that in fact this is a much more general
> problem which amounts to a lack of communication between
> get_joinrel_parampathinfo and extract_actual_join_clauses.

Yeah, I think you're right.  The rules about moving clauses down into
a parameterized path may require something that had been a regular
outer join clause to be moved to a join below its syntactic level.
If we'd done that because the clause was actually degenerate (not
mentioning the outer join's LHS), we'd have marked it is_pushed_down,
but that doesn't seem practical in this situation since the RestrictInfo
probably is also referenced in other paths where it's not pushed down.
And I don't want to start making multiple RestrictInfos for the same
clause --- that'll break some other stuff.

So the only practical answer seems to be to teach
extract_actual_join_clauses to check the clause's syntactic level
along with is_pushed_down, as per the attached patch.

Out of curiosity, I put

    Assert(bms_is_subset(rinfo->required_relids, joinrelids));

in there, and verified that we have no existing regression test
that hits the assertion, though of course Andreas' example does.

I've been pretty dissatisfied with the squishiness of is_pushed_down
for some time (cf comments in initsplan.c around line 1770), and this
bug seems like a sufficient reason to write it out of existence
entirely.  But that's surely not going to lead to a back-patchable
change, so it's likely something for v12.

            regards, tom lane

diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index b6ad01b..280f21c 100644
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
*************** create_nestloop_plan(PlannerInfo *root,
*** 3802,3807 ****
--- 3802,3808 ----
      if (IS_OUTER_JOIN(best_path->jointype))
      {
          extract_actual_join_clauses(joinrestrictclauses,
+                                     best_path->path.parent->relids,
                                      &joinclauses, &otherclauses);
      }
      else
*************** create_mergejoin_plan(PlannerInfo *root,
*** 3917,3922 ****
--- 3918,3924 ----
      if (IS_OUTER_JOIN(best_path->jpath.jointype))
      {
          extract_actual_join_clauses(joinclauses,
+                                     best_path->jpath.path.parent->relids,
                                      &joinclauses, &otherclauses);
      }
      else
*************** create_hashjoin_plan(PlannerInfo *root,
*** 4213,4218 ****
--- 4215,4221 ----
      if (IS_OUTER_JOIN(best_path->jpath.jointype))
      {
          extract_actual_join_clauses(joinclauses,
+                                     best_path->jpath.path.parent->relids,
                                      &joinclauses, &otherclauses);
      }
      else
diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optimizer/util/restrictinfo.c
index 1075dde..65c1abc 100644
*** a/src/backend/optimizer/util/restrictinfo.c
--- b/src/backend/optimizer/util/restrictinfo.c
*************** extract_actual_clauses(List *restrictinf
*** 381,386 ****
--- 381,387 ----
   */
  void
  extract_actual_join_clauses(List *restrictinfo_list,
+                             Relids joinrelids,
                              List **joinquals,
                              List **otherquals)
  {
*************** extract_actual_join_clauses(List *restri
*** 393,399 ****
      {
          RestrictInfo *rinfo = lfirst_node(RestrictInfo, l);

!         if (rinfo->is_pushed_down)
          {
              if (!rinfo->pseudoconstant)
                  *otherquals = lappend(*otherquals, rinfo->clause);
--- 394,408 ----
      {
          RestrictInfo *rinfo = lfirst_node(RestrictInfo, l);

!         /*
!          * We must check both is_pushed_down and required_relids, since an
!          * outer-join clause that's been pushed down to some lower join level
!          * via path parameterization will not be marked is_pushed_down;
!          * nonetheless, it must be treated as a filter clause not a join
!          * clause so far as the lower join level is concerned.
!          */
!         if (rinfo->is_pushed_down ||
!             !bms_is_subset(rinfo->required_relids, joinrelids))
          {
              if (!rinfo->pseudoconstant)
                  *otherquals = lappend(*otherquals, rinfo->clause);
diff --git a/src/include/optimizer/restrictinfo.h b/src/include/optimizer/restrictinfo.h
index 9cd874d..a734d79 100644
*** a/src/include/optimizer/restrictinfo.h
--- b/src/include/optimizer/restrictinfo.h
*************** extern List *get_actual_clauses(List *re
*** 36,41 ****
--- 36,42 ----
  extern List *extract_actual_clauses(List *restrictinfo_list,
                         bool pseudoconstant);
  extern void extract_actual_join_clauses(List *restrictinfo_list,
+                             Relids joinrelids,
                              List **joinquals,
                              List **otherquals);
  extern bool join_clause_is_movable_to(RestrictInfo *rinfo, RelOptInfo *baserel);