Fix pull_varnos to cope with translated PlaceHolderVars.

tglsfdc · tglsfdc · commit febe013cad59 · 2021-09-17T15:41:16.000-04:00
Commit 55dc86e changed pull_varnos to use (if possible) the associated ph_eval_at for a PlaceHolderVar. I missed a fine point though: we might be looking at a PHV in the quals or tlist of a child appendrel, in which case we need to compute a ph_eval_at value that's been translated in the same way that the PHV itself has been (cf. adjust_appendrel_attrs). Fortunately, enough info is available in the PlaceHolderInfo to make such translation possible without additional outside data, so we don't need another round of uglification of planner APIs. This is a little bit complicated, but since it's a hard-to-hit corner case, I'm not much worried about adding cycles here. Per report from Jaime Casanova. Back-patch to v12, like the previous commit. Discussion: https://postgr.es/m/20210915230959.GB17635@ahch-to
diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c
@@ -200,6 +200,16 @@ pull_varnos_walker(Node *node, pull_varnos_context *context)
 			 * join that forces delay of evaluation of a given qual clause
 			 * will be processed before we examine that clause here, so the
 			 * ph_eval_at value should have been updated to include it.
+			 *
+			 * Another problem is that a PlaceHolderVar can appear in quals or
+			 * tlists that have been translated for use in a child appendrel.
+			 * Typically such a PHV is a parameter expression sourced by some
+			 * other relation, so that the translation from parent appendrel
+			 * to child doesn't change its phrels, and we should still take
+			 * ph_eval_at at face value.  But in corner cases, the PHV's
+			 * original phrels can include the parent appendrel itself, in
+			 * which case the translated PHV will have the child appendrel in
+			 * phrels, and we must translate ph_eval_at to match.
 			 */
 			PlaceHolderInfo *phinfo = NULL;
 
@@ -215,12 +225,37 @@ pull_varnos_walker(Node *node, pull_varnos_context *context)
 					phinfo = NULL;
 				}
 			}
-			if (phinfo != NULL)
+			if (phinfo == NULL)
+			{
+				/* No PlaceHolderInfo yet, use phrels */
+				context->varnos = bms_add_members(context->varnos,
+												  phv->phrels);
+			}
+			else if (bms_equal(phv->phrels, phinfo->ph_var->phrels))
+			{
+				/* Normal case: use ph_eval_at */
 				context->varnos = bms_add_members(context->varnos,
 												  phinfo->ph_eval_at);
+			}
 			else
-				context->varnos = bms_add_members(context->varnos,
-												  phv->phrels);
+			{
+				/* Translated PlaceHolderVar: translate ph_eval_at to match */
+				Relids		newevalat,
+							delta;
+
+				/* remove what was removed from phv->phrels ... */
+				delta = bms_difference(phinfo->ph_var->phrels, phv->phrels);
+				newevalat = bms_difference(phinfo->ph_eval_at, delta);
+				/* ... then if that was in fact part of ph_eval_at ... */
+				if (!bms_equal(newevalat, phinfo->ph_eval_at))
+				{
+					/* ... add what was added */
+					delta = bms_difference(phv->phrels, phinfo->ph_var->phrels);
+					newevalat = bms_join(newevalat, delta);
+				}
+				context->varnos = bms_join(context->varnos,
+										   newevalat);
+			}
 			return false;		/* don't recurse into expression */
 		}
 	}
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
@@ -4629,6 +4629,34 @@ where q2 = 456;
  123 | 456 |   |        
 (1 row)
 
+-- and check a related issue where we miscompute required relids for
+-- a PHV that's been translated to a child rel
+create temp table parttbl (a integer primary key) partition by range (a);
+create temp table parttbl1 partition of parttbl for values from (1) to (100);
+insert into parttbl values (11), (12);
+explain (costs off)
+select * from
+  (select *, 12 as phv from parttbl) as ss
+  right join int4_tbl on true
+where ss.a = ss.phv and f1 = 0;
+         QUERY PLAN         
+----------------------------
+ Nested Loop
+   ->  Seq Scan on int4_tbl
+         Filter: (f1 = 0)
+   ->  Seq Scan on parttbl1
+         Filter: (a = 12)
+(5 rows)
+
+select * from
+  (select *, 12 as phv from parttbl) as ss
+  right join int4_tbl on true
+where ss.a = ss.phv and f1 = 0;
+ a  | phv | f1 
+----+-----+----
+ 12 |  12 |  0
+(1 row)
+
 -- bug #8444: we've historically allowed duplicate aliases within aliased JOINs
 select * from
   int8_tbl x join (int4_tbl x cross join int4_tbl y) j on q1 = f1; -- error
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
@@ -1635,6 +1635,22 @@ select i8.*, ss.v, t.unique2
     left join tenk1 t on t.unique2 = ss.v
 where q2 = 456;
 
+-- and check a related issue where we miscompute required relids for
+-- a PHV that's been translated to a child rel
+create temp table parttbl (a integer primary key) partition by range (a);
+create temp table parttbl1 partition of parttbl for values from (1) to (100);
+insert into parttbl values (11), (12);
+explain (costs off)
+select * from
+  (select *, 12 as phv from parttbl) as ss
+  right join int4_tbl on true
+where ss.a = ss.phv and f1 = 0;
+
+select * from
+  (select *, 12 as phv from parttbl) as ss
+  right join int4_tbl on true
+where ss.a = ss.phv and f1 = 0;
+
 -- bug #8444: we've historically allowed duplicate aliases within aliased JOINs
 
 select * from
