Fix filtering of "cloned" outer-join quals some more.

We've had multiple issues with the clause_is_computable_at logic that
I introduced in 2489d76: it's been known to accept more than one
clone of the same qual at the same plan node, and also to accept no
clones at all.  It's looking impractical to get it 100% right on the
basis of the currently-stored information, so fix it by introducing a
new RestrictInfo field "incompatible_relids" that explicitly shows
which outer joins a given clone mustn't be pushed above.

In principle we could populate this field in every RestrictInfo, but
that would cost space and there doesn't presently seem to be a need
for it in general.  Also, while deconstruct_distribute_oj_quals can
easily fill the field with the remaining members of the commutative
join set that it's considering, computing it in the general case
seems again pretty complicated.  So for now, just fill it for
clone quals.

Along the way, fix a bug that may or may not be only latent:
equivclass.c was generating replacement clauses with is_pushed_down
and has_clone/is_clone markings that didn't match their
required_relids.  This led me to conclude that leaving the clone flags
out of make_restrictinfo's purview wasn't such a great idea after all,
so add them.

Per report from Richard Guo.

Discussion: https://postgr.es/m/CAMbWs48EYi_9-pSd0ORes1kTmTeAjT4Q3gu49hJtYCbSn2JyeA@mail.gmail.com

Author: tglsfdc

contrib/postgres_fdw/postgres_fdw.c

@@ -6521,8 +6521,11 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel,
 									  expr,
 									  true,
 									  false,
+									  false,
+									  false,
 									  root->qual_security_level,
 									  grouped_rel->relids,
+									  NULL,
 									  NULL);
 			if (is_foreign_expr(root, grouped_rel, expr))
 				fpinfo->remote_conds = lappend(fpinfo->remote_conds, rinfo);

src/backend/optimizer/README

@@ -539,14 +539,13 @@ set includes the A/B join relid which is not in the input.  However,
 if we form B/C after A/B, then both forms of the clause are applicable
 so far as that test can tell.  We have to look more closely to notice
 that the "Pbc" clause form refers to relation B which is no longer
-directly accessible.  While this check is straightforward, it's not
-especially cheap (see clause_is_computable_at()).  To avoid doing it
-unnecessarily, we mark the variant versions of a redundant clause as
-either "has_clone" or "is_clone".  When considering a clone clause,
-we must check clause_is_computable_at() to disentangle which version
-to apply at the current join level.  (In debug builds, we also Assert
-that non-clone clauses are validly computable at the current level;
-but that seems too expensive for production usage.)
+directly accessible.  While such a check could be performed using the
+per-relation RelOptInfo.nulling_relids data, it would be annoyingly
+expensive to do over and over as we consider different join paths.
+To make this simple and reliable, we compute an "incompatible_relids"
+set for each variant version (clone) of a redundant clause.  A clone
+clause should not be applied if any of the outer-join relids listed in
+incompatible_relids has already been computed below the current join.
 
 
 Optimizer Functions

src/backend/optimizer/path/equivclass.c

@@ -197,9 +197,12 @@ process_equivalence(PlannerInfo *root,
 				make_restrictinfo(root,
 								  (Expr *) ntest,
 								  restrictinfo->is_pushed_down,
+								  restrictinfo->has_clone,
+								  restrictinfo->is_clone,
 								  restrictinfo->pseudoconstant,
 								  restrictinfo->security_level,
 								  NULL,
+								  restrictinfo->incompatible_relids,
 								  restrictinfo->outer_relids);
 		}
 		return false;
@@ -1972,7 +1975,8 @@ create_join_clause(PlannerInfo *root,
  * clause into the regular processing, because otherwise the join will be
  * seen as a clauseless join and avoided during join order searching.
  * We handle this by generating a constant-TRUE clause that is marked with
- * required_relids that make it a join between the correct relations.
+ * the same required_relids etc as the removed outer-join clause, thus
+ * making it a join clause between the correct relations.
  */
 void
 reconsider_outer_join_clauses(PlannerInfo *root)
@@ -2001,10 +2005,13 @@ reconsider_outer_join_clauses(PlannerInfo *root)
 				/* throw back a dummy replacement clause (see notes above) */
 				rinfo = make_restrictinfo(root,
 										  (Expr *) makeBoolConst(true, false),
-										  true, /* is_pushed_down */
+										  rinfo->is_pushed_down,
+										  rinfo->has_clone,
+										  rinfo->is_clone,
 										  false,	/* pseudoconstant */
 										  0,	/* security_level */
 										  rinfo->required_relids,
+										  rinfo->incompatible_relids,
 										  rinfo->outer_relids);
 				distribute_restrictinfo_to_rels(root, rinfo);
 			}
@@ -2026,10 +2033,13 @@ reconsider_outer_join_clauses(PlannerInfo *root)
 				/* throw back a dummy replacement clause (see notes above) */
 				rinfo = make_restrictinfo(root,
 										  (Expr *) makeBoolConst(true, false),
-										  true, /* is_pushed_down */
+										  rinfo->is_pushed_down,
+										  rinfo->has_clone,
+										  rinfo->is_clone,
 										  false,	/* pseudoconstant */
 										  0,	/* security_level */
 										  rinfo->required_relids,
+										  rinfo->incompatible_relids,
 										  rinfo->outer_relids);
 				distribute_restrictinfo_to_rels(root, rinfo);
 			}
@@ -2051,10 +2061,13 @@ reconsider_outer_join_clauses(PlannerInfo *root)
 				/* throw back a dummy replacement clause (see notes above) */
 				rinfo = make_restrictinfo(root,
 										  (Expr *) makeBoolConst(true, false),
-										  true, /* is_pushed_down */
+										  rinfo->is_pushed_down,
+										  rinfo->has_clone,
+										  rinfo->is_clone,
 										  false,	/* pseudoconstant */
 										  0,	/* security_level */
 										  rinfo->required_relids,
+										  rinfo->incompatible_relids,
 										  rinfo->outer_relids);
 				distribute_restrictinfo_to_rels(root, rinfo);
 			}

src/backend/optimizer/plan/initsplan.c

@@ -109,6 +109,7 @@ static void distribute_quals_to_rels(PlannerInfo *root, List *clauses,
 									 Relids qualscope,
 									 Relids ojscope,
 									 Relids outerjoin_nonnullable,
+									 Relids incompatible_relids,
 									 bool allow_equivalence,
 									 bool has_clone,
 									 bool is_clone,
@@ -120,6 +121,7 @@ static void distribute_qual_to_rels(PlannerInfo *root, Node *clause,
 									Relids qualscope,
 									Relids ojscope,
 									Relids outerjoin_nonnullable,
+									Relids incompatible_relids,
 									bool allow_equivalence,
 									bool has_clone,
 									bool is_clone,
@@ -1132,7 +1134,8 @@ deconstruct_distribute(PlannerInfo *root, JoinTreeItem *jtitem)
 								 jtitem,
 								 NULL,
 								 root->qual_security_level,
-								 jtitem->qualscope, NULL, NULL,
+								 jtitem->qualscope,
+								 NULL, NULL, NULL,
 								 true, false, false,
 								 NULL);
 
@@ -1143,7 +1146,8 @@ deconstruct_distribute(PlannerInfo *root, JoinTreeItem *jtitem)
 								 jtitem,
 								 NULL,
 								 root->qual_security_level,
-								 jtitem->qualscope, NULL, NULL,
+								 jtitem->qualscope,
+								 NULL, NULL, NULL,
 								 true, false, false,
 								 NULL);
 	}
@@ -1226,6 +1230,7 @@ deconstruct_distribute(PlannerInfo *root, JoinTreeItem *jtitem)
 								 root->qual_security_level,
 								 jtitem->qualscope,
 								 ojscope, jtitem->nonnullable_rels,
+								 NULL,	/* incompatible_relids */
 								 true,	/* allow_equivalence */
 								 false, false,	/* not clones */
 								 postponed_oj_qual_list);
@@ -1285,6 +1290,7 @@ process_security_barrier_quals(PlannerInfo *root,
 								 jtitem->qualscope,
 								 jtitem->qualscope,
 								 NULL,
+								 NULL,
 								 true,
 								 false, false,	/* not clones */
 								 NULL);
@@ -1887,6 +1893,7 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
 	{
 		Relids		joins_above;
 		Relids		joins_below;
+		Relids		incompatible_joins;
 		Relids		joins_so_far;
 		List	   *quals;
 		int			save_last_rinfo_serial;
@@ -1920,6 +1927,15 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
 												   joins_below,
 												   NULL);
 
+		/*
+		 * We'll need to mark the lower versions of the quals as not safe to
+		 * apply above not-yet-processed joins of the stack.  This prevents
+		 * possibly applying a cloned qual at the wrong join level.
+		 */
+		incompatible_joins = bms_union(joins_below, joins_above);
+		incompatible_joins = bms_add_member(incompatible_joins,
+											sjinfo->ojrelid);
+
 		/*
 		 * Each time we produce RestrictInfo(s) from these quals, reset the
 		 * last_rinfo_serial counter, so that the RestrictInfos for the "same"
@@ -1979,13 +1995,19 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
 			 * relation B will appear nulled by the syntactically-upper OJ
 			 * within the Pbc clause, but those of relation C will not.  (In
 			 * the notation used by optimizer/README, we're converting a qual
-			 * of the form Pbc to Pb*c.)
+			 * of the form Pbc to Pb*c.)  Of course, we must also remove that
+			 * bit from the incompatible_joins value, else we'll make a qual
+			 * that can't be placed anywhere.
 			 */
 			if (above_sjinfo)
+			{
 				quals = (List *)
 					add_nulling_relids((Node *) quals,
 									   sjinfo->syn_lefthand,
 									   bms_make_singleton(othersj->ojrelid));
+				incompatible_joins = bms_del_member(incompatible_joins,
+													othersj->ojrelid);
+			}
 
 			/* Compute qualscope and ojscope for this join level */
 			this_qualscope = bms_union(qualscope, joins_so_far);
@@ -2027,6 +2049,7 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
 									 root->qual_security_level,
 									 this_qualscope,
 									 this_ojscope, nonnullable_rels,
+									 bms_copy(incompatible_joins),
 									 allow_equivalence,
 									 has_clone,
 									 is_clone,
@@ -2039,13 +2062,17 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
 			 * Vars coming from the lower join's RHS.  (Again, we are
 			 * converting a qual of the form Pbc to Pb*c, but now we are
 			 * putting back bits that were there in the parser output and were
-			 * temporarily stripped above.)
+			 * temporarily stripped above.)  Update incompatible_joins too.
 			 */
 			if (below_sjinfo)
+			{
 				quals = (List *)
 					add_nulling_relids((Node *) quals,
 									   othersj->syn_righthand,
 									   bms_make_singleton(othersj->ojrelid));
+				incompatible_joins = bms_del_member(incompatible_joins,
+													othersj->ojrelid);
+			}
 
 			/* ... and track joins processed so far */
 			joins_so_far = bms_add_member(joins_so_far, othersj->ojrelid);
@@ -2060,6 +2087,7 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
 								 root->qual_security_level,
 								 qualscope,
 								 ojscope, nonnullable_rels,
+								 NULL,	/* incompatible_relids */
 								 true,	/* allow_equivalence */
 								 false, false,	/* not clones */
 								 NULL); /* no more postponement */
@@ -2086,6 +2114,7 @@ distribute_quals_to_rels(PlannerInfo *root, List *clauses,
 						 Relids qualscope,
 						 Relids ojscope,
 						 Relids outerjoin_nonnullable,
+						 Relids incompatible_relids,
 						 bool allow_equivalence,
 						 bool has_clone,
 						 bool is_clone,
@@ -2104,6 +2133,7 @@ distribute_quals_to_rels(PlannerInfo *root, List *clauses,
 								qualscope,
 								ojscope,
 								outerjoin_nonnullable,
+								incompatible_relids,
 								allow_equivalence,
 								has_clone,
 								is_clone,
@@ -2135,6 +2165,9 @@ distribute_quals_to_rels(PlannerInfo *root, List *clauses,
  *		base+OJ rels appearing on the outer (nonnullable) side of the join
  *		(for FULL JOIN this includes both sides of the join, and must in fact
  *		equal qualscope)
+ * 'incompatible_relids': the set of outer-join relid(s) that must not be
+ *		computed below this qual.  We only bother to compute this for
+ *		"clone" quals, otherwise it can be left NULL.
  * 'allow_equivalence': true if it's okay to convert clause into an
  *		EquivalenceClass
  * 'has_clone': has_clone property to assign to the qual
@@ -2159,6 +2192,7 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause,
 						Relids qualscope,
 						Relids ojscope,
 						Relids outerjoin_nonnullable,
+						Relids incompatible_relids,
 						bool allow_equivalence,
 						bool has_clone,
 						bool is_clone,
@@ -2377,15 +2411,14 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause,
 	restrictinfo = make_restrictinfo(root,
 									 (Expr *) clause,
 									 is_pushed_down,
+									 has_clone,
+									 is_clone,
 									 pseudoconstant,
 									 security_level,
 									 relids,
+									 incompatible_relids,
 									 outerjoin_nonnullable);
 
-	/* Apply appropriate clone marking, too */
-	restrictinfo->has_clone = has_clone;
-	restrictinfo->is_clone = is_clone;
-
 	/*
 	 * If it's a join clause, add vars used in the clause to targetlists of
 	 * their relations, so that they will be emitted by the plan nodes that
@@ -2750,9 +2783,12 @@ process_implied_equality(PlannerInfo *root,
 	restrictinfo = make_restrictinfo(root,
 									 (Expr *) clause,
 									 true,	/* is_pushed_down */
+									 false, /* !has_clone */
+									 false, /* !is_clone */
 									 pseudoconstant,
 									 security_level,
 									 relids,
+									 NULL,	/* incompatible_relids */
 									 NULL); /* outer_relids */
 
 	/*
@@ -2841,9 +2877,12 @@ build_implied_join_equality(PlannerInfo *root,
 	restrictinfo = make_restrictinfo(root,
 									 clause,
 									 true,	/* is_pushed_down */
+									 false, /* !has_clone */
+									 false, /* !is_clone */
 									 false, /* pseudoconstant */
 									 security_level,	/* security_level */
 									 qualscope, /* required_relids */
+									 NULL,	/* incompatible_relids */
 									 NULL); /* outer_relids */
 
 	/* Set mergejoinability/hashjoinability flags */

src/backend/optimizer/util/inherit.c

@@ -894,9 +894,11 @@ apply_child_basequals(PlannerInfo *root, RelOptInfo *parentrel,
 								 make_restrictinfo(root,
 												   (Expr *) onecq,
 												   rinfo->is_pushed_down,
+												   rinfo->has_clone,
+												   rinfo->is_clone,
 												   pseudoconstant,
 												   rinfo->security_level,
-												   NULL, NULL));
+												   NULL, NULL, NULL));
 			/* track minimum security level among child quals */
 			cq_min_security = Min(cq_min_security, rinfo->security_level);
 		}
@@ -929,9 +931,11 @@ apply_child_basequals(PlannerInfo *root, RelOptInfo *parentrel,
 				/* not likely that we'd see constants here, so no check */
 				childquals = lappend(childquals,
 									 make_restrictinfo(root, qual,
-													   true, false,
+													   true,
+													   false, false,
+													   false,
 													   security_level,
-													   NULL, NULL));
+													   NULL, NULL, NULL));
 				cq_min_security = Min(cq_min_security, security_level);
 			}
 			security_level++;

src/backend/optimizer/util/orclauses.c

@@ -267,8 +267,11 @@ consider_new_or_clause(PlannerInfo *root, RelOptInfo *rel,
 								 orclause,
 								 true,
 								 false,
+								 false,
+								 false,
 								 join_or_rinfo->security_level,
 								 NULL,
+								 NULL,
 								 NULL);
 
 	/*

src/backend/optimizer/util/relnode.c

@@ -1346,27 +1346,21 @@ subbuild_joinrel_restrictlist(PlannerInfo *root,
 				Assert(!RINFO_IS_PUSHED_DOWN(rinfo, joinrel->relids));
 				if (!bms_is_subset(rinfo->required_relids, both_input_relids))
 					continue;
-				if (!clause_is_computable_at(root, rinfo, both_input_relids))
+				if (bms_overlap(rinfo->incompatible_relids, both_input_relids))
 					continue;
 			}
 			else
 			{
 				/*
 				 * For non-clone clauses, we just Assert it's OK.  These might
-				 * be either join or filter clauses.
+				 * be either join or filter clauses; if it's a join clause
+				 * then it should not refer to the current join's output.
+				 * (There is little point in checking incompatible_relids,
+				 * because it'll be NULL.)
 				 */
-#ifdef USE_ASSERT_CHECKING
-				if (RINFO_IS_PUSHED_DOWN(rinfo, joinrel->relids))
-					Assert(clause_is_computable_at(root, rinfo,
-												   joinrel->relids));
-				else
-				{
-					Assert(bms_is_subset(rinfo->required_relids,
-										 both_input_relids));
-					Assert(clause_is_computable_at(root, rinfo,
-												   both_input_relids));
-				}
-#endif
+				Assert(RINFO_IS_PUSHED_DOWN(rinfo, joinrel->relids) ||
+					   bms_is_subset(rinfo->required_relids,
+									 both_input_relids));
 			}
 
 			/*