Fix PlaceHolderVar mechanism's interaction with outer joins.

The point of a PlaceHolderVar is to allow a non-strict expression to be
evaluated below an outer join, after which its value bubbles up like a Var
and can be forced to NULL when the outer join's semantics require that.
However, there was a serious design oversight in that, namely that we
didn't ensure that there was actually a correct place in the plan tree
to evaluate the placeholder :-(.  It may be necessary to delay evaluation
of an outer join to ensure that a placeholder that should be evaluated
below the join can be evaluated there.  Per recent bug report from Kirill
Simonov.

Back-patch to 8.4 where the PlaceHolderVar mechanism was introduced.

src/backend/nodes/copyfuncs.c

@@ -1806,6 +1806,7 @@ _copyPlaceHolderInfo(PlaceHolderInfo *from)
 	COPY_NODE_FIELD(ph_var);
 	COPY_BITMAPSET_FIELD(ph_eval_at);
 	COPY_BITMAPSET_FIELD(ph_needed);
+	COPY_BITMAPSET_FIELD(ph_may_need);
 	COPY_SCALAR_FIELD(ph_width);
 
 	return newnode;

src/backend/nodes/equalfuncs.c

@@ -838,6 +838,7 @@ _equalPlaceHolderInfo(PlaceHolderInfo *a, PlaceHolderInfo *b)
 	COMPARE_NODE_FIELD(ph_var);
 	COMPARE_BITMAPSET_FIELD(ph_eval_at);
 	COMPARE_BITMAPSET_FIELD(ph_needed);
+	COMPARE_BITMAPSET_FIELD(ph_may_need);
 	COMPARE_SCALAR_FIELD(ph_width);
 
 	return true;

src/backend/nodes/outfuncs.c

@@ -1768,6 +1768,7 @@ _outPlaceHolderInfo(StringInfo str, PlaceHolderInfo *node)
 	WRITE_NODE_FIELD(ph_var);
 	WRITE_BITMAPSET_FIELD(ph_eval_at);
 	WRITE_BITMAPSET_FIELD(ph_needed);
+	WRITE_BITMAPSET_FIELD(ph_may_need);
 	WRITE_INT_FIELD(ph_width);
 }
 

src/backend/optimizer/plan/analyzejoins.c

@@ -396,6 +396,8 @@ remove_rel_from_query(PlannerInfo *root, int relid, Relids joinrelids)
 			phinfo->ph_eval_at = bms_add_member(phinfo->ph_eval_at, relid);
 
 		phinfo->ph_needed = bms_del_member(phinfo->ph_needed, relid);
+		/* ph_may_need probably isn't used after this, but fix it anyway */
+		phinfo->ph_may_need = bms_del_member(phinfo->ph_may_need, relid);
 	}
 
 	/*

src/backend/optimizer/plan/initsplan.c

@@ -187,6 +187,13 @@ add_vars_to_targetlist(PlannerInfo *root, List *vars, Relids where_needed)
 
 			phinfo->ph_needed = bms_add_members(phinfo->ph_needed,
 												where_needed);
+			/*
+			 * Update ph_may_need too.  This is currently only necessary
+			 * when being called from build_base_rel_tlists, but we may as
+			 * well do it always.
+			 */
+			phinfo->ph_may_need = bms_add_members(phinfo->ph_may_need,
+												  where_needed);
 		}
 		else
 			elog(ERROR, "unrecognized node type: %d", (int) nodeTag(node));
@@ -465,7 +472,11 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode, bool below_outer_join,
 
 		/* Now we can add the SpecialJoinInfo to join_info_list */
 		if (sjinfo)
+		{
 			root->join_info_list = lappend(root->join_info_list, sjinfo);
+			/* Each time we do that, recheck placeholder eval levels */
+			update_placeholder_eval_levels(root, sjinfo);
+		}
 
 		/*
 		 * Finally, compute the output joinlist.  We fold subproblems together
@@ -687,6 +698,32 @@ make_outerjoininfo(PlannerInfo *root,
 		}
 	}
 
+	/*
+	 * Examine PlaceHolderVars.  If a PHV is supposed to be evaluated within
+	 * this join's nullable side, and it may get used above this join, then
+	 * ensure that min_righthand contains the full eval_at set of the PHV.
+	 * This ensures that the PHV actually can be evaluated within the RHS.
+	 * Note that this works only because we should already have determined
+	 * the final eval_at level for any PHV syntactically within this join.
+	 */
+	foreach(l, root->placeholder_list)
+	{
+		PlaceHolderInfo *phinfo = (PlaceHolderInfo *) lfirst(l);
+		Relids		ph_syn_level = phinfo->ph_var->phrels;
+
+		/* Ignore placeholder if it didn't syntactically come from RHS */
+		if (!bms_is_subset(ph_syn_level, right_rels))
+			continue;
+
+		/* We can also ignore it if it's certainly not used above this join */
+		/* XXX this test is probably overly conservative */
+		if (bms_is_subset(phinfo->ph_may_need, min_righthand))
+			continue;
+
+		/* Else, prevent join from being formed before we eval the PHV */
+		min_righthand = bms_add_members(min_righthand, phinfo->ph_eval_at);
+	}
+
 	/*
 	 * If we found nothing to put in min_lefthand, punt and make it the full
 	 * LHS, to avoid having an empty min_lefthand which will confuse later

src/backend/optimizer/plan/planmain.c

@@ -180,14 +180,19 @@ query_planner(PlannerInfo *root, List *tlist,
 	add_base_rels_to_query(root, (Node *) parse->jointree);
 
 	/*
-	 * Examine the targetlist and qualifications, adding entries to baserel
-	 * targetlists for all referenced Vars.  Restrict and join clauses are
-	 * added to appropriate lists belonging to the mentioned relations.  We
-	 * also build EquivalenceClasses for provably equivalent expressions, and
-	 * form a target joinlist for make_one_rel() to work from.
+	 * Examine the targetlist and join tree, adding entries to baserel
+	 * targetlists for all referenced Vars, and generating PlaceHolderInfo
+	 * entries for all referenced PlaceHolderVars.  Restrict and join clauses
+	 * are added to appropriate lists belonging to the mentioned relations.
+	 * We also build EquivalenceClasses for provably equivalent expressions.
+	 * The SpecialJoinInfo list is also built to hold information about join
+	 * order restrictions.  Finally, we form a target joinlist for
+	 * make_one_rel() to work from.
 	 */
 	build_base_rel_tlists(root, tlist);
 
+	find_placeholders_in_jointree(root);
+
 	joinlist = deconstruct_jointree(root);
 
 	/*
@@ -218,10 +223,12 @@ query_planner(PlannerInfo *root, List *tlist,
 
 	/*
 	 * Examine any "placeholder" expressions generated during subquery pullup.
-	 * Make sure that we know what level to evaluate them at, and that the
-	 * Vars they need are marked as needed.
+	 * Make sure that the Vars they need are marked as needed at the relevant
+	 * join level.  This must be done before join removal because it might
+	 * cause Vars or placeholders to be needed above a join when they weren't
+	 * so marked before.
 	 */
-	fix_placeholder_eval_levels(root);
+	fix_placeholder_input_needed_levels(root);
 
 	/*
 	 * Remove any useless outer joins.	Ideally this would be done during

src/include/nodes/relation.h

@@ -1316,8 +1316,22 @@ typedef struct AppendRelInfo
  * then allow it to bubble up like a Var until the ph_needed join level.
  * ph_needed has the same definition as attr_needed for a regular Var.
  *
+ * ph_may_need is an initial estimate of ph_needed, formed using the
+ * syntactic locations of references to the PHV.  We need this in order to
+ * determine whether the PHV reference forces a join ordering constraint:
+ * if the PHV has to be evaluated below the nullable side of an outer join,
+ * and then used above that outer join, we must constrain join order to ensure
+ * there's a valid place to evaluate the PHV below the join.  The final
+ * actual ph_needed level might be lower than ph_may_need, but we can't
+ * determine that until later on.  Fortunately this doesn't matter for what
+ * we need ph_may_need for: if there's a PHV reference syntactically
+ * above the outer join, it's not going to be allowed to drop below the outer
+ * join, so we would come to the same conclusions about join order even if
+ * we had the final ph_needed value to compare to.
+ *
  * We create a PlaceHolderInfo only after determining that the PlaceHolderVar
- * is actually referenced in the plan tree.
+ * is actually referenced in the plan tree, so that unreferenced placeholders
+ * don't result in unnecessary constraints on join order.
  */
 
 typedef struct PlaceHolderInfo
@@ -1328,6 +1342,7 @@ typedef struct PlaceHolderInfo
 	PlaceHolderVar *ph_var;		/* copy of PlaceHolderVar tree */
 	Relids		ph_eval_at;		/* lowest level we can evaluate value at */
 	Relids		ph_needed;		/* highest level the value is needed at */
+	Relids		ph_may_need;	/* highest level it might be needed at */
 	int32		ph_width;		/* estimated attribute width */
 } PlaceHolderInfo;
 

src/include/optimizer/placeholder.h

@@ -21,7 +21,10 @@ extern PlaceHolderVar *make_placeholder_expr(PlannerInfo *root, Expr *expr,
 					  Relids phrels);
 extern PlaceHolderInfo *find_placeholder_info(PlannerInfo *root,
 					  PlaceHolderVar *phv);
-extern void fix_placeholder_eval_levels(PlannerInfo *root);
+extern void find_placeholders_in_jointree(PlannerInfo *root);
+extern void update_placeholder_eval_levels(PlannerInfo *root,
+										   SpecialJoinInfo *new_sjinfo);
+extern void fix_placeholder_input_needed_levels(PlannerInfo *root);
 extern void add_placeholders_to_base_rels(PlannerInfo *root);
 extern void add_placeholders_to_joinrel(PlannerInfo *root,
 							RelOptInfo *joinrel);

src/test/regress/expected/join.out

@@ -2443,6 +2443,51 @@ group by t1.q2 order by 1;
   4567890123456789 |     6
 (4 rows)
 
+--
+-- test incorrect failure to NULL pulled-up subexpressions
+--
+begin;
+create temp table a (
+     code char not null,
+     constraint a_pk primary key (code)
+);
+NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "a_pk" for table "a"
+create temp table b (
+     a char not null,
+     num integer not null,
+     constraint b_pk primary key (a, num)
+);
+NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "b_pk" for table "b"
+create temp table c (
+     name char not null,
+     a char,
+     constraint c_pk primary key (name)
+);
+NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "c_pk" for table "c"
+insert into a (code) values ('p');
+insert into a (code) values ('q');
+insert into b (a, num) values ('p', 1);
+insert into b (a, num) values ('p', 2);
+insert into c (name, a) values ('A', 'p');
+insert into c (name, a) values ('B', 'q');
+insert into c (name, a) values ('C', null);
+select c.name, ss.code, ss.b_cnt, ss.const
+from c left join
+  (select a.code, coalesce(b_grp.cnt, 0) as b_cnt, -1 as const
+   from a left join
+     (select count(1) as cnt, b.a from b group by b.a) as b_grp
+     on a.code = b_grp.a
+  ) as ss
+  on (c.a = ss.code)
+order by c.name;
+ name | code | b_cnt | const 
+------+------+-------+-------
+ A    | p    |     2 |    -1
+ B    | q    |     0 |    -1
+ C    |      |       |      
+(3 rows)
+
+rollback;
 --
 -- test the corner cases FULL JOIN ON TRUE and FULL JOIN ON FALSE
 --

src/test/regress/sql/join.sql

@@ -562,6 +562,46 @@ from int8_tbl t1 left join
   on (t1.q2 = t2.q1)
 group by t1.q2 order by 1;
 
+--
+-- test incorrect failure to NULL pulled-up subexpressions
+--
+begin;
+
+create temp table a (
+     code char not null,
+     constraint a_pk primary key (code)
+);
+create temp table b (
+     a char not null,
+     num integer not null,
+     constraint b_pk primary key (a, num)
+);
+create temp table c (
+     name char not null,
+     a char,
+     constraint c_pk primary key (name)
+);
+
+insert into a (code) values ('p');
+insert into a (code) values ('q');
+insert into b (a, num) values ('p', 1);
+insert into b (a, num) values ('p', 2);
+insert into c (name, a) values ('A', 'p');
+insert into c (name, a) values ('B', 'q');
+insert into c (name, a) values ('C', null);
+
+select c.name, ss.code, ss.b_cnt, ss.const
+from c left join
+  (select a.code, coalesce(b_grp.cnt, 0) as b_cnt, -1 as const
+   from a left join
+     (select count(1) as cnt, b.a from b group by b.a) as b_grp
+     on a.code = b_grp.a
+  ) as ss
+  on (c.a = ss.code)
+order by c.name;
+
+rollback;
+
 --
 -- test the corner cases FULL JOIN ON TRUE and FULL JOIN ON FALSE
 --