Re: BUG #19106: Potential regression with CTE materialization planning in Postgres 18 - Mailing list pgsql-bugs

Richard Guo <guofenglinux@gmail.com> writes:
> However, I still find this behavior somewhat confusing.  For example,
> one might expect that an inlined CTE should be semantically equivalent
> to a subquery, yet the following two queries can produce different
> results.

> create table t (a int);
> insert into t values (1), (2);

> with ss as not materialized (select * from t)
> select (select sum((select a from ss where a = t.a limit 1))) from t;
>  sum
> -----
>    1
>    2
> (2 rows)

> select (select sum((select a from (select * from t) ss where a = t.a
> limit 1))) from t;
>  sum
> -----
>    3
> (1 row)

That's with my v1 patch?  I agree it's pretty bogus.

The more I think about this, the more I like the other idea of just
throwing an error rather than trying to fix up cases like bug #19055.
I don't think we have much evidence that anyone is trying to do that
in the real world (otherwise reports would have surfaced years ago).
And this discussion is making it clear that fixing it up is harder
than it sounds.

Also: if we throw an error, and someone shows up with a real-world
example that triggers the error, we'd then have some more evidence
about what would be a plausible interpretation.

So now I'm thinking about something more like the attached.  It
causes the #19055 query to throw an error, and restores our current
test query to working (I added said query to the test so we can't
break it again).

Producing a decently localized error turned out to be harder than
I expected, because RangeTableEntrys don't have location fields so
it's impossible to put an error cursor on the troublesome reference.
I ended up putting the cursor on the aggregate function itself
and giving the name of the CTE in errdetail.

            regards, tom lane

diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 3254c83cc6c..b8340557b34 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -38,6 +38,8 @@ typedef struct
     ParseState *pstate;
     int            min_varlevel;
     int            min_agglevel;
+    int            min_ctelevel;
+    RangeTblEntry *min_cte;
     int            sublevels_up;
 } check_agg_arguments_context;

@@ -58,7 +60,8 @@ typedef struct
 static int    check_agg_arguments(ParseState *pstate,
                                 List *directargs,
                                 List *args,
-                                Expr *filter);
+                                Expr *filter,
+                                int agglocation);
 static bool check_agg_arguments_walker(Node *node,
                                        check_agg_arguments_context *context);
 static Node *substitute_grouped_columns(Node *node, ParseState *pstate, Query *qry,
@@ -339,7 +342,8 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
     min_varlevel = check_agg_arguments(pstate,
                                        directargs,
                                        args,
-                                       filter);
+                                       filter,
+                                       location);

     *p_levelsup = min_varlevel;

@@ -641,7 +645,8 @@ static int
 check_agg_arguments(ParseState *pstate,
                     List *directargs,
                     List *args,
-                    Expr *filter)
+                    Expr *filter,
+                    int agglocation)
 {
     int            agglevel;
     check_agg_arguments_context context;
@@ -649,6 +654,8 @@ check_agg_arguments(ParseState *pstate,
     context.pstate = pstate;
     context.min_varlevel = -1;    /* signifies nothing found yet */
     context.min_agglevel = -1;
+    context.min_ctelevel = -1;
+    context.min_cte = NULL;
     context.sublevels_up = 0;

     (void) check_agg_arguments_walker((Node *) args, &context);
@@ -686,6 +693,20 @@ check_agg_arguments(ParseState *pstate,
                  parser_errposition(pstate, aggloc)));
     }

+    /*
+     * If there's a non-local CTE that's below the aggregate's semantic level,
+     * complain.  It's not quite clear what we should do to fix up such a case
+     * (treating the CTE reference like a Var seems wrong), and it's also
+     * unclear whether there is a real-world use for such cases.
+     */
+    if (context.min_ctelevel >= 0 && context.min_ctelevel < agglevel)
+        ereport(ERROR,
+                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                 errmsg("outer-level aggregate cannot use a nested CTE"),
+                 errdetail("CTE \"%s\" is below the aggregate's semantic level.",
+                           context.min_cte->eref->aliasname),
+                 parser_errposition(pstate, agglocation)));
+
     /*
      * Now check for vars/aggs in the direct arguments, and throw error if
      * needed.  Note that we allow a Var of the agg's semantic level, but not
@@ -699,6 +720,7 @@ check_agg_arguments(ParseState *pstate,
     {
         context.min_varlevel = -1;
         context.min_agglevel = -1;
+        context.min_ctelevel = -1;
         (void) check_agg_arguments_walker((Node *) directargs, &context);
         if (context.min_varlevel >= 0 && context.min_varlevel < agglevel)
             ereport(ERROR,
@@ -714,6 +736,13 @@ check_agg_arguments(ParseState *pstate,
                      parser_errposition(pstate,
                                         locate_agg_of_level((Node *) directargs,
                                                             context.min_agglevel))));
+        if (context.min_ctelevel >= 0 && context.min_ctelevel < agglevel)
+            ereport(ERROR,
+                    (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                     errmsg("outer-level aggregate cannot use a nested CTE"),
+                     errdetail("CTE \"%s\" is below the aggregate's semantic level.",
+                               context.min_cte->eref->aliasname),
+                     parser_errposition(pstate, agglocation)));
     }
     return agglevel;
 }
@@ -794,11 +823,6 @@ check_agg_arguments_walker(Node *node,

     if (IsA(node, RangeTblEntry))
     {
-        /*
-         * CTE references act similarly to Vars of the CTE's level.  Without
-         * this we might conclude that the Agg can be evaluated above the CTE,
-         * leading to trouble.
-         */
         RangeTblEntry *rte = (RangeTblEntry *) node;

         if (rte->rtekind == RTE_CTE)
@@ -810,9 +834,12 @@ check_agg_arguments_walker(Node *node,
             /* ignore local CTEs of subqueries */
             if (ctelevelsup >= 0)
             {
-                if (context->min_varlevel < 0 ||
-                    context->min_varlevel > ctelevelsup)
-                    context->min_varlevel = ctelevelsup;
+                if (context->min_ctelevel < 0 ||
+                    context->min_ctelevel > ctelevelsup)
+                {
+                    context->min_ctelevel = ctelevelsup;
+                    context->min_cte = rte;
+                }
             }
         }
         return false;            /* allow range_table_walker to continue */
diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out
index 86fdb85c6c5..f4caedf272f 100644
--- a/src/test/regress/expected/with.out
+++ b/src/test/regress/expected/with.out
@@ -2300,36 +2300,44 @@ from int4_tbl;
 --
 -- test for bug #19055: interaction of WITH with aggregates
 --
--- The reference to cte1 must determine the aggregate's level,
--- even though it contains no Vars referencing cte1
-explain (verbose, costs off)
+-- For now, we just throw an error if there's a use of a CTE below the
+-- semantic level that the SQL standard assigns to the aggregate.
+-- It's not entirely clear what we could do instead that doesn't risk
+-- breaking more things than it fixes.
 select f1, (with cte1(x,y) as (select 1,2)
             select count((select i4.f1 from cte1))) as ss
 from int4_tbl i4;
-                   QUERY PLAN
--------------------------------------------------
- Seq Scan on public.int4_tbl i4
-   Output: i4.f1, (SubPlan expr_1)
-   SubPlan expr_1
+ERROR:  outer-level aggregate cannot use a nested CTE
+LINE 2:             select count((select i4.f1 from cte1))) as ss
+                           ^
+DETAIL:  CTE "cte1" is below the aggregate's semantic level.
+--
+-- test for bug #19106: interaction of WITH with aggregates
+--
+-- the initial fix for #19055 was too aggressive and broke this case
+explain (verbose, costs off)
+with a as ( select id from (values (1), (2)) as v(id) ),
+     b as ( select max((select sum(id) from a)) as agg )
+select agg from b;
+                 QUERY PLAN
+--------------------------------------------
+ Aggregate
+   Output: max((InitPlan expr_1).col1)
+   InitPlan expr_1
      ->  Aggregate
-           Output: count((InitPlan expr_2).col1)
-           InitPlan expr_2
-             ->  Result
-                   Output: i4.f1
-           ->  Result
-(9 rows)
+           Output: sum("*VALUES*".column1)
+           ->  Values Scan on "*VALUES*"
+                 Output: "*VALUES*".column1
+   ->  Result
+(8 rows)

-select f1, (with cte1(x,y) as (select 1,2)
-            select count((select i4.f1 from cte1))) as ss
-from int4_tbl i4;
-     f1      | ss
--------------+----
-           0 |  1
-      123456 |  1
-     -123456 |  1
-  2147483647 |  1
- -2147483647 |  1
-(5 rows)
+with a as ( select id from (values (1), (2)) as v(id) ),
+     b as ( select max((select sum(id) from a)) as agg )
+select agg from b;
+ agg
+-----
+   3
+(1 row)

 --
 -- test for nested-recursive-WITH bug
diff --git a/src/test/regress/sql/with.sql b/src/test/regress/sql/with.sql
index d88d5abb91a..cd25a5e7154 100644
--- a/src/test/regress/sql/with.sql
+++ b/src/test/regress/sql/with.sql
@@ -1100,16 +1100,26 @@ from int4_tbl;
 --
 -- test for bug #19055: interaction of WITH with aggregates
 --
--- The reference to cte1 must determine the aggregate's level,
--- even though it contains no Vars referencing cte1
-explain (verbose, costs off)
+-- For now, we just throw an error if there's a use of a CTE below the
+-- semantic level that the SQL standard assigns to the aggregate.
+-- It's not entirely clear what we could do instead that doesn't risk
+-- breaking more things than it fixes.
 select f1, (with cte1(x,y) as (select 1,2)
             select count((select i4.f1 from cte1))) as ss
 from int4_tbl i4;

-select f1, (with cte1(x,y) as (select 1,2)
-            select count((select i4.f1 from cte1))) as ss
-from int4_tbl i4;
+--
+-- test for bug #19106: interaction of WITH with aggregates
+--
+-- the initial fix for #19055 was too aggressive and broke this case
+explain (verbose, costs off)
+with a as ( select id from (values (1), (2)) as v(id) ),
+     b as ( select max((select sum(id) from a)) as agg )
+select agg from b;
+
+with a as ( select id from (values (1), (2)) as v(id) ),
+     b as ( select max((select sum(id) from a)) as agg )
+select agg from b;

 --
 -- test for nested-recursive-WITH bug