From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: xinwen@stu.scu.edu.cn, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17835: Two assertions failed in nodeAgg.c and
 execExprInterp.c with the same SQL
In-reply-to: <1772189.1678721174@sss.pgh.pa.us>
References: <17835-4f29f3098b2d0ba4@postgresql.org>
 <1772189.1678721174@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Mon, 13 Mar 2023 11:26:14 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1885576.1678723973.0@sss.pgh.pa.us>
Date: Mon, 13 Mar 2023 12:13:25 -0400
Message-ID: <1885605.1678724005@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1885605.1678724005%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1885576.1678723973.1@sss.pgh.pa.us>

I wrote:
> v10 detected this in ExecInitAgg, while later versions are trying to
> do it in the parser, but evidently there's some gap there ...

Looking closer, 69c3936a1 didn't really break this, it just exposed
that it was already broken.  check_agg_arguments() has never gotten
this case right, but it was back-stopped by the check in ExecInitAgg,
which 69c3936a1 removed because it was no longer easy to check there.

The problem is simple: check_agg_arguments supposes that an inner
(lower level) aggregate cannot contain aggregates of outer levels,
which is just wrong on its face.  So we can't skip descending into
an aggregate's arguments.  Somebody (probably Gierth) recognized
that this was true for grouping functions, but didn't see that the
adjacent code was buggy.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-nested-agg-detection.patch";
	charset="us-ascii"
Content-ID: <1885576.1678723973.2@sss.pgh.pa.us>
Content-Description: fix-nested-agg-detection.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg=
.c
index 4fbf80c271..85cd47b7ae 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -736,8 +736,7 @@ check_agg_arguments_walker(Node *node,
 				context->min_agglevel > agglevelsup)
 				context->min_agglevel =3D agglevelsup;
 		}
-		/* no need to examine args of the inner aggregate */
-		return false;
+		/* Continue and descend into subtree */
 	}
 	if (IsA(node, GroupingFunc))
 	{
diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/e=
xpected/aggregates.out
index e074cb71bf..f0517f95b6 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -1282,6 +1282,12 @@ select (select max(min(unique1)) from int8_tbl) fro=
m tenk1;
 ERROR:  aggregate function calls cannot be nested
 LINE 1: select (select max(min(unique1)) from int8_tbl) from tenk1;
                            ^
+select avg((select avg(a1.col1 order by (select avg(a2.col2) from tenk1 a=
3))
+            from tenk1 a1(col1)))
+from tenk1 a2(col2);
+ERROR:  aggregate function calls cannot be nested
+LINE 1: select avg((select avg(a1.col1 order by (select avg(a2.col2)...
+                                                        ^
 --
 -- Test removal of redundant GROUP BY columns
 --
diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/ag=
gregates.sql
index 616ef38c25..1783d19bd5 100644
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -439,6 +439,9 @@ drop table minmaxtest cascade;
 -- check for correct detection of nested-aggregate errors
 select max(min(unique1)) from tenk1;
 select (select max(min(unique1)) from int8_tbl) from tenk1;
+select avg((select avg(a1.col1 order by (select avg(a2.col2) from tenk1 a=
3))
+            from tenk1 a1(col1)))
+from tenk1 a2(col2);
 =

 --
 -- Test removal of redundant GROUP BY columns

------- =_aaaaaaaaaa0--