Fix handling of changed-Param signaling for CteScan plan nodes. We were using

the "cteParam" as a proxy for the possibility that the underlying CTE plan
depends on outer-level variables or Params, but that doesn't work very well
because it sometimes causes calling subqueries to be treated as SubPlans when
they could be InitPlans.  This is inefficient and also causes the outright
failure exhibited in bug #4902.  Instead, leave the cteParam out of it and
copy the underlying CTE plan's extParams directly.  Per bug #4902 from
Marko Tiikkaja.

diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
index cdff123828172b01335aa2e4d11b5003ea768f6c..f8d11a4714f89f236b6e9401bae7858c85d10257 100644 (file)
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/optimizer/plan/subselect.c,v 1.150 2009/06/11 14:48:59 momjian Exp $
+ *       $PostgreSQL: pgsql/src/backend/optimizer/plan/subselect.c,v 1.151 2009/07/06 02:16:03 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1916,9 +1916,35 @@ finalize_plan(PlannerInfo *root, Plan *plan, Bitmapset *valid_params)
                        break;
 
                case T_CteScan:
-                       context.paramids =
-                               bms_add_member(context.paramids,
-                                                          ((CteScan *) plan)->cteParam);
+                       {
+                               /*
+                                * You might think we should add the node's cteParam to
+                                * paramids, but we shouldn't because that param is just a
+                                * linkage mechanism for multiple CteScan nodes for the same
+                                * CTE; it is never used for changed-param signaling.  What
+                                * we have to do instead is to find the referenced CTE plan
+                                * and incorporate its external paramids, so that the correct
+                                * things will happen if the CTE references outer-level
+                                * variables.  See test cases for bug #4902.
+                                */
+                               int             plan_id = ((CteScan *) plan)->ctePlanId;
+                               Plan   *cteplan;
+
+                               /* so, do this ... */
+                               if (plan_id < 1 || plan_id > list_length(root->glob->subplans))
+                                       elog(ERROR, "could not find plan for CteScan referencing plan ID %d",
+                                                plan_id);
+                               cteplan = (Plan *) list_nth(root->glob->subplans, plan_id - 1);
+                               context.paramids =
+                                       bms_add_members(context.paramids, cteplan->extParam);
+
+#ifdef NOT_USED
+                               /* ... but not this */
+                               context.paramids =
+                                       bms_add_member(context.paramids,
+                                                                  ((CteScan *) plan)->cteParam);
+#endif
+                       }
                        break;
 
                case T_WorkTableScan:

diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out
index 4a2f18c9e9beb1a0b784e10e433c0f70ba776098..98003ebe6c68d11eff7f1824df5428408d20a77c 100644 (file)
--- a/src/test/regress/expected/with.out
+++ b/src/test/regress/expected/with.out
@@ -912,3 +912,44 @@ ERROR:  recursive query "foo" column 1 has type numeric(3,0) in non-recursive te
 LINE 2:    (SELECT i::numeric(3,0) FROM (VALUES(1),(2)) t(i)
                    ^
 HINT:  Cast the output of the non-recursive term to the correct type.
+--
+-- test for bug #4902
+--
+with cte(foo) as ( values(42) ) values((select foo from cte));
+ column1 
+---------
+      42
+(1 row)
+
+with cte(foo) as ( select 42 ) select * from ((select foo from cte)) q;
+ foo 
+-----
+  42
+(1 row)
+
+-- test CTE referencing an outer-level variable (to see that changed-parameter
+-- signaling still works properly after fixing this bug)
+select ( with cte(foo) as ( values(f1) )
+         select (select foo from cte) )
+from int4_tbl;
+  ?column?   
+-------------
+           0
+      123456
+     -123456
+  2147483647
+ -2147483647
+(5 rows)
+
+select ( with cte(foo) as ( values(f1) )
+          values((select foo from cte)) )
+from int4_tbl;
+  ?column?   
+-------------
+           0
+      123456
+     -123456
+  2147483647
+ -2147483647
+(5 rows)
+

diff --git a/src/test/regress/sql/with.sql b/src/test/regress/sql/with.sql
index c736441f5365dbd41e977c12c7f38ea58d1db153..6eaa168d964db3355350e863a4f17c97f7d15caf 100644 (file)
--- a/src/test/regress/sql/with.sql
+++ b/src/test/regress/sql/with.sql
@@ -469,3 +469,19 @@ WITH RECURSIVE foo(i) AS
    UNION ALL
    SELECT (i+1)::numeric(10,0) FROM foo WHERE i < 10)
 SELECT * FROM foo;
+
+--
+-- test for bug #4902
+--
+with cte(foo) as ( values(42) ) values((select foo from cte));
+with cte(foo) as ( select 42 ) select * from ((select foo from cte)) q;
+
+-- test CTE referencing an outer-level variable (to see that changed-parameter
+-- signaling still works properly after fixing this bug)
+select ( with cte(foo) as ( values(f1) )
+         select (select foo from cte) )
+from int4_tbl;
+
+select ( with cte(foo) as ( values(f1) )
+          values((select foo from cte)) )
+from int4_tbl;