Remove over-optimistic Assert.

tglsfdc · tglsfdc · commit eae0e20deffb · 2023-01-31T11:57:47.000-05:00
In commit 2489d76, I'd thought it'd be safe to assert that a PlaceHolderVar appearing in a scan-level expression has empty nullingrels. However this is not so, as when we determine that a join relation is certainly empty we'll put its targetlist into a Result-with-constant-false-qual node, and nothing is done to adjust the nullingrels of the Vars or PHVs therein. (Arguably, a Result used in this way isn't really a scan-level node, but it certainly isn't an upper node either ...) It's not clear this is worth any close analysis, so let's just take out the faulty Assert. Per report from Robins Tharakan. I added a test case based on his example, just in case somebody tries to tighten this up. Discussion: https://postgr.es/m/CAEP4nAz7Enq3+DEthGG7j27DpuwSRZnW0Nh6jtNh75yErQ_nbA@mail.gmail.com
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
@@ -2205,7 +2205,7 @@ fix_scan_expr_mutator(Node *node, fix_scan_expr_context *context)
 		/* At scan level, we should always just evaluate the contained expr */
 		PlaceHolderVar *phv = (PlaceHolderVar *) node;
 
-		Assert(phv->phnullingrels == NULL);
+		/* XXX can we assert something about phnullingrels? */
 		return fix_scan_expr_mutator((Node *) phv->phexpr, context);
 	}
 	if (IsA(node, AlternativeSubPlan))
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
@@ -3599,6 +3599,20 @@ where f1 in ( select c3.f1 from ctetable c2 full join ctetable c3 on true );
   1
 (1 row)
 
+-- Test PHV that winds up in a Result node, despite having nonempty nullingrels
+explain (verbose, costs off)
+select table_catalog, table_name
+from int4_tbl t1
+  inner join (int8_tbl t2
+              left join information_schema.column_udt_usage on null)
+  on null;
+                                                    QUERY PLAN                                                     
+-------------------------------------------------------------------------------------------------------------------
+ Result
+   Output: (current_database())::information_schema.sql_identifier, (c.relname)::information_schema.sql_identifier
+   One-Time Filter: false
+(3 rows)
+
 --
 -- test inlining of immutable functions
 --
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
@@ -1181,6 +1181,14 @@ with ctetable as not materialized ( select 1 as f1 )
 select * from ctetable c1
 where f1 in ( select c3.f1 from ctetable c2 full join ctetable c3 on true );
 
+-- Test PHV that winds up in a Result node, despite having nonempty nullingrels
+explain (verbose, costs off)
+select table_catalog, table_name
+from int4_tbl t1
+  inner join (int8_tbl t2
+              left join information_schema.column_udt_usage on null)
+  on null;
+
 --
 -- test inlining of immutable functions
 --

Original file line number	Diff line number	Diff line change
`@@ -2205,7 +2205,7 @@ fix_scan_expr_mutator(Node node, fix_scan_expr_context context)`
`2205`	`2205`	`/* At scan level, we should always just evaluate the contained expr */`
`2206`	`2206`	`PlaceHolderVar phv = (PlaceHolderVar ) node;`
`2207`	`2207`
`2208`		`- Assert(phv->phnullingrels == NULL);`
	`2208`	`+ /* XXX can we assert something about phnullingrels? */`
`2209`	`2209`	`return fix_scan_expr_mutator((Node *) phv->phexpr, context);`
`2210`	`2210`	`}`
`2211`	`2211`	`if (IsA(node, AlternativeSubPlan))`