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
Date: Wed, 18 Aug 2021 14:01:46 -0400
Message-ID: <1495661.1629309706@sss.pgh.pa.us> (raw)
In-Reply-To: <1431890.1629300960@sss.pgh.pa.us>
References: <17152-c7f906cc1a88e61b@postgresql.org>
<1431890.1629300960@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
Attachments:fix-detection-of-nested-aggs-in-FILTER.patch (text/x-diff, 1.2K) downloaddiff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.cindex 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),
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, 253540651@qq.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17152: ERROR: AddressSanitizer: SEGV on unknown address
In-Reply-To: <1495661.1629309706@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox