From: Tom Lane <tgl@sss.pgh.pa.us>
To: 253540651@qq.com
cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17152: ERROR: AddressSanitizer: SEGV on unknown address
In-reply-to: <1431890.1629300960@sss.pgh.pa.us>
References: <17152-c7f906cc1a88e61b@postgresql.org>
 <1431890.1629300960@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Wed, 18 Aug 2021 11:36:00 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1495551.1629309585.0@sss.pgh.pa.us>
Date: Wed, 18 Aug 2021 14:01:46 -0400
Message-ID: <1495661.1629309706@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1495661.1629309706%40sss.pgh.pa.us>
Precedence: bulk

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

I wrote:
> So somehow the fact that outer references are involved is misleading that
> error check.  I've not traced it further than that.

Actually, it's simpler than that.  The patch that added FILTER
taught check_agg_arguments to check the FILTER argument like this:

    (void) expression_tree_walker((Node *) filter,
                                  check_agg_arguments_walker,
                                  (void *) &context);

which was a blind copy-and-paste of what it does for the "args"
list.  However, that coding for "args" includes an undocumented
optimization: "args" is a List, and check_agg_arguments_walker
doesn't care about Lists, so it's okay to recurse directly to
expression_tree_walker without invoking the walker on the top-level
List node.  This is completely NOT okay for the "filter" argument,
which could be directly a Var or Aggref.  Hence, the check completely
misses a top-level Var or Aggref in FILTER.

It would be enough to convert this one call to

    (void) check_agg_arguments_walker((Node *) filter, &context);

but in the attached I just converted all three of check_agg_arguments'
invocations to look that way.  The few nanoseconds shaved by the
other coding don't justify the risk of more copy-and-paste bugs.

An interesting nearby issue is that check_agglevels_and_constraints is
not really handling this correctly either.  It supposes that once it's
identified the agglevelsup for an Aggref node, it can look that many
levels up in the parse stack to see whether it's inside a relevant
FILTER expression.  This is wrong, because we've not yet determined
the semantic level of the surrounding aggregate if any, so we've
certainly not set p_expr_kind in the relevant outer pstate level.
That explains why you get

regression=# CREATE TEMP TABLE v0 ( v2 int );
CREATE TABLE
regression=#                                 
 SELECT
  MODE ( ) WITHIN GROUP ( ORDER BY v2 )
    FILTER ( WHERE MODE ( ) WITHIN GROUP ( ORDER BY v2 > 0 ) )
FROM v0;
ERROR:  aggregate functions are not allowed in FILTER
LINE 3:     FILTER ( WHERE MODE ( ) WITHIN GROUP ( ORDER BY v2 > 0 )...
                           ^

while (after this patch) the outer-level case gives

regression=# SELECT (                        
 SELECT
  MODE ( ) WITHIN GROUP ( ORDER BY v2 )
    FILTER ( WHERE MODE ( ) WITHIN GROUP ( ORDER BY v2 > 0 ) )
)
FROM v0;
ERROR:  aggregate function calls cannot be nested
LINE 4:     FILTER ( WHERE MODE ( ) WITHIN GROUP ( ORDER BY v2 > 0 )...
                           ^

check_agglevels_and_constraints doesn't complain at the lower
Aggref, so it's left to the upper one to whine.

I'm not too concerned about this cosmetic difference, but I wonder
if there are any cases where check_agglevels_and_constraints will
throw an error when it shouldn't.  Right offhand I can't see any,
but I may just lack imagination today.

Regardless of that, we certainly need the attached patch (and,
probably, some more regression tests).

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="fix-detection-of-nested-aggs-in-FILTER.patch";
	charset="us-ascii"
Content-ID: <1495551.1629309585.2@sss.pgh.pa.us>
Content-Description: fix-detection-of-nested-aggs-in-FILTER.patch

diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 24268eb502..7d829a05a9 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -627,13 +627,8 @@ check_agg_arguments(ParseState *pstate,
 	context.min_agglevel = -1;
 	context.sublevels_up = 0;
 
-	(void) expression_tree_walker((Node *) args,
-								  check_agg_arguments_walker,
-								  (void *) &context);
-
-	(void) expression_tree_walker((Node *) filter,
-								  check_agg_arguments_walker,
-								  (void *) &context);
+	(void) check_agg_arguments_walker((Node *) args, &context);
+	(void) check_agg_arguments_walker((Node *) filter, &context);
 
 	/*
 	 * If we found no vars nor aggs at all, it's a level-zero aggregate;
@@ -680,9 +675,7 @@ check_agg_arguments(ParseState *pstate,
 	{
 		context.min_varlevel = -1;
 		context.min_agglevel = -1;
-		(void) expression_tree_walker((Node *) directargs,
-									  check_agg_arguments_walker,
-									  (void *) &context);
+		(void) check_agg_arguments_walker((Node *) directargs, &context);
 		if (context.min_varlevel >= 0 && context.min_varlevel < agglevel)
 			ereport(ERROR,
 					(errcode(ERRCODE_GROUPING_ERROR),

------- =_aaaaaaaaaa0--