1 files changed, 32 insertions, 7 deletions

diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optimizer/util/restrictinfo.c
index e5f78365175..65499902f6c 100644
--- a/src/backend/optimizer/util/restrictinfo.c
+++ b/src/backend/optimizer/util/restrictinfo.c
@@ -464,10 +464,9 @@ extract_actual_join_clauses(List *restrictinfo_list,
  * outer join, as that would change the results (rows would be suppressed
  * rather than being null-extended).
  *
- * Also the target relation must not be in the clause's nullable_relids, i.e.,
- * there must not be an outer join below the clause that would null the Vars
- * coming from the target relation.  Otherwise the clause might give results
- * different from what it would give at its normal semantic level.
+ * Also there must not be an outer join below the clause that would null the
+ * Vars coming from the target relation.  Otherwise the clause might give
+ * results different from what it would give at its normal semantic level.
  *
  * Also, the join clause must not use any relations that have LATERAL
  * references to the target relation, since we could not put such rels on
@@ -516,10 +515,31 @@ join_clause_is_movable_to(RestrictInfo *rinfo, RelOptInfo *baserel)
  * not pushing the clause into its outer-join outer side, nor down into
  * a lower outer join's inner side.
  *
+ * The check about pushing a clause down into a lower outer join's inner side
+ * is only approximate; it sometimes returns "false" when actually it would
+ * be safe to use the clause here because we're still above the outer join
+ * in question.  This is okay as long as the answers at different join levels
+ * are consistent: it just means we might sometimes fail to push a clause as
+ * far down as it could safely be pushed.  It's unclear whether it would be
+ * worthwhile to do this more precisely.  (But if it's ever fixed to be
+ * exactly accurate, there's an Assert in get_joinrel_parampathinfo() that
+ * should be re-enabled.)
+ *
  * There's no check here equivalent to join_clause_is_movable_to's test on
  * lateral_referencers.  We assume the caller wouldn't be inquiring unless
  * it'd verified that the proposed outer rels don't have lateral references
- * to the current rel(s).
+ * to the current rel(s).  (If we are considering join paths with the outer
+ * rels on the outside and the current rels on the inside, then this should
+ * have been checked at the outset of such consideration; see join_is_legal
+ * and the path parameterization checks in joinpath.c.)  On the other hand,
+ * in join_clause_is_movable_to we are asking whether the clause could be
+ * moved for some valid set of outer rels, so we don't have the benefit of
+ * relying on prior checks for lateral-reference validity.
+ *
+ * Note: if this returns true, it means that the clause could be moved to
+ * this join relation, but that doesn't mean that this is the lowest join
+ * it could be moved to.  Caller may need to make additional calls to verify
+ * that this doesn't succeed on either of the inputs of a proposed join.
  *
  * Note: get_joinrel_parampathinfo depends on the fact that if
  * current_and_outer is NULL, this function will always return false
@@ -534,7 +554,7 @@ join_clause_is_movable_into(RestrictInfo *rinfo,
 	if (!bms_is_subset(rinfo->clause_relids, current_and_outer))
 		return false;
 
-	/* Clause must physically reference target rel(s) */
+	/* Clause must physically reference at least one target rel */
 	if (!bms_overlap(currentrelids, rinfo->clause_relids))
 		return false;
 
@@ -542,7 +562,12 @@ join_clause_is_movable_into(RestrictInfo *rinfo,
 	if (bms_overlap(currentrelids, rinfo->outer_relids))
 		return false;
 
-	/* Target rel(s) must not be nullable below the clause */
+	/*
+	 * Target rel(s) must not be nullable below the clause.  This is
+	 * approximate, in the safe direction, because the current join might be
+	 * above the join where the nulling would happen, in which case the clause
+	 * would work correctly here.  But we don't have enough info to be sure.
+	 */
 	if (bms_overlap(currentrelids, rinfo->nullable_relids))
 		return false;