pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #17088: FailedAssertion in prepagg.c
12+ messages / 6 participants
[nested] [flat]
* BUG #17088: FailedAssertion in prepagg.c
@ 2021-07-07 06:33 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 12+ messages in thread
From: PG Bug reporting form @ 2021-07-07 06:33 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: cyg0810@gmail.com
The following bug has been logged on the website:
Bug reference: 17088
Logged by: yaoguang chen
Email address: cyg0810@gmail.com
PostgreSQL version: 14beta2
Operating system: Linux supersix 5.4.0-39-generic #43-Ubuntu SMP Fri
Description:
run the following sql command through client and the PostgreSQL database
process will crash:
CREATE TEMP TABLE v0 ( v1 INT PRIMARY KEY ) ON COMMIT DELETE ROWS ;
SELECT FROM ( VALUES ( ( SELECT - - 84 FROM v0 LIMIT - -1 ) ) ) v1 ( v1 )
GROUP BY ROLLUP ( v1 , v1 ) , ROLLUP ( ROW ( ) , ROW ( - - - - -128 ,
6099928.000000 ) , v1 ) ORDER BY v1 = v1 AND v1 = - - ( SELECT GROUPING ( v1
) GROUP BY v1 ) ASC FETCH FIRST ROWS WITH TIES
crash log:
HINT: Future log output will go to log destination "csvlog".
TRAP: FailedAssertion("!IsA(node, SubLink)", File:
"/home/supersix/fuzz/Squirrel/PostgreSQL/postgres/build/../src/backend/optimizer/prep/prepagg.c",
Line: 341, PID: 2310031)
postgres: supersix x 127.0.0.1(65126)
SELECT(ExceptionalCondition+0xbb)[0x562398112ffb]
postgres: supersix x 127.0.0.1(65126)
SELECT(preprocess_aggrefs+0x0)[0x562397d2da10]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x95)[0x562397c8c755]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x641)[0x562397c8cd01]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x95)[0x562397c8c755]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x7ef)[0x562397c8ceaf]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x95)[0x562397c8c755]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x159)[0x562397c8c819]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x95)[0x562397c8c755]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x159)[0x562397c8c819]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x95)[0x562397c8c755]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x159)[0x562397c8c819]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x95)[0x562397c8c755]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x832)[0x562397c8cef2]
postgres: supersix x 127.0.0.1(65126)
SELECT(expression_tree_walker+0x95)[0x562397c8c755]
postgres: supersix x 127.0.0.1(65126) SELECT(+0x592dd9)[0x562397d1bdd9]
postgres: supersix x 127.0.0.1(65126)
SELECT(subquery_planner+0xf63)[0x562397d1e8e3]
postgres: supersix x 127.0.0.1(65126)
SELECT(standard_planner+0x165)[0x562397d1f535]
postgres: supersix x 127.0.0.1(65126)
SELECT(pg_plan_query+0x6a)[0x562397ebceaa]
postgres: supersix x 127.0.0.1(65126)
SELECT(pg_plan_queries+0x4d)[0x562397ebcffd]
postgres: supersix x 127.0.0.1(65126) SELECT(+0x7359f2)[0x562397ebe9f2]
postgres: supersix x 127.0.0.1(65126)
SELECT(PostgresMain+0x1ae7)[0x562397ec0d57]
postgres: supersix x 127.0.0.1(65126) SELECT(+0x61671f)[0x562397d9f71f]
postgres: supersix x 127.0.0.1(65126)
SELECT(PostmasterMain+0x1182)[0x562397da2672]
postgres: supersix x 127.0.0.1(65126) SELECT(main+0x533)[0x562397852133]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3)[0x7f6aa088f0b3]
postgres: supersix x 127.0.0.1(65126) SELECT(_start+0x2e)[0x56239785228e]
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17088: FailedAssertion in prepagg.c
@ 2021-07-07 11:00 Michael Paquier <michael@paquier.xyz>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 12+ messages in thread
From: Michael Paquier @ 2021-07-07 11:00 UTC (permalink / raw)
To: cyg0810@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Jul 07, 2021 at 06:33:07AM +0000, PG Bug reporting form wrote:
> run the following sql command through client and the PostgreSQL database
> process will crash:
>
> CREATE TEMP TABLE v0 ( v1 INT PRIMARY KEY ) ON COMMIT DELETE ROWS ;
> SELECT FROM ( VALUES ( ( SELECT - - 84 FROM v0 LIMIT - -1 ) ) ) v1 ( v1 )
> GROUP BY ROLLUP ( v1 , v1 ) , ROLLUP ( ROW ( ) , ROW ( - - - - -128 ,
> 6099928.000000 ) , v1 ) ORDER BY v1 = v1 AND v1 = - - ( SELECT GROUPING ( v1
> ) GROUP BY v1 ) ASC FETCH FIRST ROWS WITH TIES
Reproduced here, thanks for the test case. As far as I can see, this
is not limited to 14.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../YOWJQTovI%2F%2FDNe8%2F@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17088: FailedAssertion in prepagg.c
@ 2021-07-07 20:49 Tom Lane <tgl@sss.pgh.pa.us>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 12+ messages in thread
From: Tom Lane @ 2021-07-07 20:49 UTC (permalink / raw)
To: Andrew Gierth <andrew@tao11.riddles.org.uk>; +Cc: Michael Paquier <michael@paquier.xyz>; cyg0810@gmail.com; pgsql-bugs@lists.postgresql.org
Michael Paquier <michael@paquier.xyz> writes:
>> CREATE TEMP TABLE v0 ( v1 INT PRIMARY KEY ) ON COMMIT DELETE ROWS ;
>> SELECT FROM ( VALUES ( ( SELECT - - 84 FROM v0 LIMIT - -1 ) ) ) v1 ( v1 )
>> GROUP BY ROLLUP ( v1 , v1 ) , ROLLUP ( ROW ( ) , ROW ( - - - - -128 ,
>> 6099928.000000 ) , v1 ) ORDER BY v1 = v1 AND v1 = - - ( SELECT GROUPING ( v1
>> ) GROUP BY v1 ) ASC FETCH FIRST ROWS WITH TIES
> Reproduced here, thanks for the test case. As far as I can see, this
> is not limited to 14.
Yeah, this looks like it probably dates back to the addition of
GroupingFunc. The test case can be simplified a good deal:
SELECT (SELECT GROUPING(v1)) FROM (VALUES ((SELECT 1))) v(v1) GROUP BY cube(v1);
server closed the connection unexpectedly
I also found a probably-related variant:
SELECT (SELECT GROUPING(v1)) FROM (VALUES ((SELECT 1))) v(v1) GROUP BY v1;
ERROR: plan should not reference subplan's variable
These cases don't fail if the GROUPING call isn't inside a sub-select.
The proximate cause of the assertion failure is that preprocess_aggrefs
isn't expecting to find a SubLink, which is reasonable since we should
have removed them already. However, what it's actually seeing is
{TARGETENTRY
:expr
{SUBPLAN
...
:args (
{GROUPINGFUNC
:args (
{PLACEHOLDERVAR
:phexpr
{SUBLINK
...
}
...
If we don't put GROUPING(v1) inside a sub-SELECT, it looks like
{GROUPINGFUNC
:args (
{PLACEHOLDERVAR
:phexpr
{PARAM
:paramkind 1
:paramid 0
:paramtype 23
:paramtypmod -1
:paramcollid 0
:location -1
}
:phrels (b 2)
:phid 1
:phlevelsup 0
}
)
:refs (i 1)
:cols <>
:agglevelsup 0
:location 15
}
which seems a whole lot saner. So I surmise that somebody is
missing doing something relevant to the "args" list of a SubPlan.
An alternative theory is that we should never have done anything
at all to the argument tree of a GroupingFunc. Since it's not
supposed to be evaluated, treating it as a target for expression
preprocessing might be a mistake altogether. I wonder why its
arguments aren't stored as sortgroupref indexes or the like.
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17088: FailedAssertion in prepagg.c
@ 2021-07-07 21:18 Andrew Gierth <andrew@tao11.riddles.org.uk>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Gierth @ 2021-07-07 21:18 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Michael Paquier <michael@paquier.xyz>; cyg0810@gmail.com; pgsql-bugs@lists.postgresql.org
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
Tom> An alternative theory is that we should never have done anything
Tom> at all to the argument tree of a GroupingFunc. Since it's not
Tom> supposed to be evaluated, treating it as a target for expression
Tom> preprocessing might be a mistake altogether. I wonder why its
Tom> arguments aren't stored as sortgroupref indexes or the like.
The arguments are stored as sortgrouprefs (the "refs" list) for actual
use; the "args" list is only kept for the benefit of EXPLAIN and
deparsing.
A number of places in the planner have to explicitly avoid recursing
into GroupingFunc->args when walking trees specifically because they are
not evaluated. It looks to me like some places where that should have
been checked for were missed. Looking into it.
--
Andrew (irc:RhodiumToad)
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17088: FailedAssertion in prepagg.c
@ 2021-07-07 21:22 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andrew Gierth <andrew@tao11.riddles.org.uk>
0 siblings, 1 reply; 12+ messages in thread
From: Tom Lane @ 2021-07-07 21:22 UTC (permalink / raw)
To: Andrew Gierth <andrew@tao11.riddles.org.uk>; +Cc: Michael Paquier <michael@paquier.xyz>; cyg0810@gmail.com; pgsql-bugs@lists.postgresql.org
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> A number of places in the planner have to explicitly avoid recursing
> into GroupingFunc->args when walking trees specifically because they are
> not evaluated. It looks to me like some places where that should have
> been checked for were missed. Looking into it.
Hmm. Maybe it'd be better if the default behavior in
expression_tree_walker/mutator did not include recursing into the args,
then? I am thinking this might be comparable to SubLinks/SubPlans, where
the walker has to take explicit action if it wants to recurse into the
sub-query.
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17088: FailedAssertion in prepagg.c
@ 2021-07-07 21:32 Andrew Gierth <andrew@tao11.riddles.org.uk>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Gierth @ 2021-07-07 21:32 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Michael Paquier <michael@paquier.xyz>; cyg0810@gmail.com; pgsql-bugs@lists.postgresql.org
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
>> A number of places in the planner have to explicitly avoid recursing
>> into GroupingFunc->args when walking trees specifically because they
>> are not evaluated. It looks to me like some places where that should
>> have been checked for were missed. Looking into it.
Tom> Hmm. Maybe it'd be better if the default behavior in
Tom> expression_tree_walker/mutator did not include recursing into the
Tom> args, then?
You'd think, but as I recall (I will re-check this to confirm) there
were more places where we _did_ need to recurse (especially during parse
analysis before we've matched up the sortgrouprefs), while most of the
places where recursion needed to be explicitly avoided already needed
special-case handling, so having the default the other way would likely
have required a special-case almost everywhere.
--
Andrew.
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17088: FailedAssertion in prepagg.c
@ 2021-07-07 21:56 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andrew Gierth <andrew@tao11.riddles.org.uk>
0 siblings, 1 reply; 12+ messages in thread
From: Tom Lane @ 2021-07-07 21:56 UTC (permalink / raw)
To: Andrew Gierth <andrew@tao11.riddles.org.uk>; +Cc: Michael Paquier <michael@paquier.xyz>; cyg0810@gmail.com; pgsql-bugs@lists.postgresql.org
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> Tom> Hmm. Maybe it'd be better if the default behavior in
> Tom> expression_tree_walker/mutator did not include recursing into the
> Tom> args, then?
> You'd think, but as I recall (I will re-check this to confirm) there
> were more places where we _did_ need to recurse (especially during parse
> analysis before we've matched up the sortgrouprefs), while most of the
> places where recursion needed to be explicitly avoided already needed
> special-case handling, so having the default the other way would likely
> have required a special-case almost everywhere.
Fair enough. This is the kind of design choice that can be worth
revisiting later; but if the conclusion is still the same, fine with me.
regards, tom lane
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17088: FailedAssertion in prepagg.c
@ 2021-07-08 05:44 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 12+ messages in thread
From: Richard Guo @ 2021-07-08 05:44 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Andrew Gierth <andrew@tao11.riddles.org.uk>; Michael Paquier <michael@paquier.xyz>; cyg0810@gmail.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On Thu, Jul 8, 2021 at 5:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> > "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> > Tom> Hmm. Maybe it'd be better if the default behavior in
> > Tom> expression_tree_walker/mutator did not include recursing into the
> > Tom> args, then?
>
> > You'd think, but as I recall (I will re-check this to confirm) there
> > were more places where we _did_ need to recurse (especially during parse
> > analysis before we've matched up the sortgrouprefs), while most of the
> > places where recursion needed to be explicitly avoided already needed
> > special-case handling, so having the default the other way would likely
> > have required a special-case almost everywhere.
>
> Fair enough. This is the kind of design choice that can be worth
> revisiting later; but if the conclusion is still the same, fine with me.
>
I think the culprit is that when replacing correlation uplevel vars with
Params, we do not handle the SubLinks in the arguments of uplevel
GroupingFunc. We expect build_subplan should take care of it. But in
build_subplan, we ignore GroupingFunc incorrectly.
diff --git a/src/backend/optimizer/plan/subselect.c
b/src/backend/optimizer/plan/subselect.c
index 0881a208ac..e4918f275e 100644
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -364,7 +364,8 @@ build_subplan(PlannerInfo *root, Plan *plan,
PlannerInfo *subroot,
* SS_replace_correlation_vars). Do that now.
*/
if (IsA(arg, PlaceHolderVar) ||
- IsA(arg, Aggref))
+ IsA(arg, Aggref) ||
+ IsA(arg, GroupingFunc))
arg = SS_process_sublinks(root, arg, false);
Thanks
Richard
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17088: FailedAssertion in prepagg.c
@ 2021-07-08 06:14 Richard Guo <guofenglinux@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Richard Guo @ 2021-07-08 06:14 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Andrew Gierth <andrew@tao11.riddles.org.uk>; Michael Paquier <michael@paquier.xyz>; cyg0810@gmail.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On Thu, Jul 8, 2021 at 1:44 PM Richard Guo <guofenglinux@gmail.com> wrote:
>
> On Thu, Jul 8, 2021 at 5:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
>> > "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>> > Tom> Hmm. Maybe it'd be better if the default behavior in
>> > Tom> expression_tree_walker/mutator did not include recursing into the
>> > Tom> args, then?
>>
>> > You'd think, but as I recall (I will re-check this to confirm) there
>> > were more places where we _did_ need to recurse (especially during parse
>> > analysis before we've matched up the sortgrouprefs), while most of the
>> > places where recursion needed to be explicitly avoided already needed
>> > special-case handling, so having the default the other way would likely
>> > have required a special-case almost everywhere.
>>
>> Fair enough. This is the kind of design choice that can be worth
>> revisiting later; but if the conclusion is still the same, fine with me.
>>
>
> I think the culprit is that when replacing correlation uplevel vars with
> Params, we do not handle the SubLinks in the arguments of uplevel
> GroupingFunc. We expect build_subplan should take care of it. But in
> build_subplan, we ignore GroupingFunc incorrectly.
>
> diff --git a/src/backend/optimizer/plan/subselect.c
> b/src/backend/optimizer/plan/subselect.c
> index 0881a208ac..e4918f275e 100644
> --- a/src/backend/optimizer/plan/subselect.c
> +++ b/src/backend/optimizer/plan/subselect.c
> @@ -364,7 +364,8 @@ build_subplan(PlannerInfo *root, Plan *plan,
> PlannerInfo *subroot,
> * SS_replace_correlation_vars). Do that now.
> */
> if (IsA(arg, PlaceHolderVar) ||
> - IsA(arg, Aggref))
> + IsA(arg, Aggref) ||
> + IsA(arg, GroupingFunc))
> arg = SS_process_sublinks(root, arg, false);
>
>
I think we also need to change SS_process_sublinks to avoid recursing
into the arguments of an outer GroupingFunc. And that leads to a fix as
attached.
Thanks
Richard
Attachments:
[application/octet-stream] 0001-Fix-GroupingFunc-assertion-failure.patch (1.2K, ../../CAMbWs4_1m1Xd2JfCHFjGk9=Pj1gTiicYwG1JEpKLaxLub19RXA@mail.gmail.com/3-0001-Fix-GroupingFunc-assertion-failure.patch)
download | inline diff:
From c52167cef7acd9f9b0146f7d4b28bad5545e7996 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Thu, 8 Jul 2021 14:07:15 +0800
Subject: [PATCH] Fix GroupingFunc assertion failure
---
src/backend/optimizer/plan/subselect.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
index 0881a208ac..264c2fc496 100644
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -364,7 +364,8 @@ build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot,
* SS_replace_correlation_vars). Do that now.
*/
if (IsA(arg, PlaceHolderVar) ||
- IsA(arg, Aggref))
+ IsA(arg, Aggref) ||
+ IsA(arg, GroupingFunc))
arg = SS_process_sublinks(root, arg, false);
splan->parParam = lappend_int(splan->parParam, pitem->paramId);
@@ -1974,6 +1975,11 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context)
if (((Aggref *) node)->agglevelsup > 0)
return node;
}
+ else if (IsA(node, GroupingFunc))
+ {
+ if (((GroupingFunc *) node)->agglevelsup > 0)
+ return node;
+ }
/*
* We should never see a SubPlan expression in the input (since this is
--
2.31.0
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17088: FailedAssertion in prepagg.c
@ 2021-07-09 06:54 Richard Guo <guofenglinux@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Richard Guo @ 2021-07-09 06:54 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Andrew Gierth <andrew@tao11.riddles.org.uk>; Michael Paquier <michael@paquier.xyz>; cyg0810@gmail.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On Thu, Jul 8, 2021 at 2:14 PM Richard Guo <guofenglinux@gmail.com> wrote:
>
> On Thu, Jul 8, 2021 at 1:44 PM Richard Guo <guofenglinux@gmail.com> wrote:
>
>>
>> I think the culprit is that when replacing correlation uplevel vars with
>> Params, we do not handle the SubLinks in the arguments of uplevel
>> GroupingFunc. We expect build_subplan should take care of it. But in
>> build_subplan, we ignore GroupingFunc incorrectly.
>>
>> diff --git a/src/backend/optimizer/plan/subselect.c
>> b/src/backend/optimizer/plan/subselect.c
>> index 0881a208ac..e4918f275e 100644
>> --- a/src/backend/optimizer/plan/subselect.c
>> +++ b/src/backend/optimizer/plan/subselect.c
>> @@ -364,7 +364,8 @@ build_subplan(PlannerInfo *root, Plan *plan,
>> PlannerInfo *subroot,
>> * SS_replace_correlation_vars). Do that now.
>> */
>> if (IsA(arg, PlaceHolderVar) ||
>> - IsA(arg, Aggref))
>> + IsA(arg, Aggref) ||
>> + IsA(arg, GroupingFunc))
>> arg = SS_process_sublinks(root, arg, false);
>>
>>
>
> I think we also need to change SS_process_sublinks to avoid recursing
> into the arguments of an outer GroupingFunc. And that leads to a fix as
> attached.
>
Update the patch with comments and test cases.
Thanks
Richard
Attachments:
[application/octet-stream] v1-0001-Fix-handling-of-outer-GroupingFunc-within-subqueries.patch (4.9K, ../../CAMbWs4_zpvPa1+qGBU78FhQqiQo_RNDK2vwP3uBxs-S-mABxJQ@mail.gmail.com/3-v1-0001-Fix-handling-of-outer-GroupingFunc-within-subqueries.patch)
download | inline diff:
From 94a800434b670725a24e41a74ca6fcbb0ad6989a Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Fri, 9 Jul 2021 13:52:09 +0800
Subject: [PATCH] Fix handling of outer GroupingFunc within subqueries
---
src/backend/optimizer/plan/subselect.c | 28 +++++++++-----
src/test/regress/expected/groupingsets.out | 45 ++++++++++++++++++++++
src/test/regress/sql/groupingsets.sql | 9 +++++
3 files changed, 72 insertions(+), 10 deletions(-)
diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
index 0881a208ac..2cae6cefc1 100644
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -356,15 +356,17 @@ build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot,
Node *arg = pitem->item;
/*
- * The Var, PlaceHolderVar, or Aggref has already been adjusted to
- * have the correct varlevelsup, phlevelsup, or agglevelsup.
+ * The Var, PlaceHolderVar, Aggref or GroupingFunc has already been
+ * adjusted to have the correct varlevelsup, phlevelsup, or
+ * agglevelsup.
*
- * If it's a PlaceHolderVar or Aggref, its arguments might contain
- * SubLinks, which have not yet been processed (see the comments for
- * SS_replace_correlation_vars). Do that now.
+ * If it's a PlaceHolderVar, Aggref or GroupingFunc, its arguments
+ * might contain SubLinks, which have not yet been processed (see the
+ * comments for SS_replace_correlation_vars). Do that now.
*/
if (IsA(arg, PlaceHolderVar) ||
- IsA(arg, Aggref))
+ IsA(arg, Aggref) ||
+ IsA(arg, GroupingFunc))
arg = SS_process_sublinks(root, arg, false);
splan->parParam = lappend_int(splan->parParam, pitem->paramId);
@@ -1959,10 +1961,11 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context)
}
/*
- * Don't recurse into the arguments of an outer PHV or aggregate here. Any
- * SubLinks in the arguments have to be dealt with at the outer query
- * level; they'll be handled when build_subplan collects the PHV or Aggref
- * into the arguments to be passed down to the current subplan.
+ * Don't recurse into the arguments of an outer PHV, aggregate or
+ * GroupingFunc here. Any SubLinks in the arguments have to be dealt with
+ * at the outer query level; they'll be handled when build_subplan collects
+ * the PHV, Aggref or GroupingFunc into the arguments to be passed down to
+ * the current subplan.
*/
if (IsA(node, PlaceHolderVar))
{
@@ -1974,6 +1977,11 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context)
if (((Aggref *) node)->agglevelsup > 0)
return node;
}
+ else if (IsA(node, GroupingFunc))
+ {
+ if (((GroupingFunc *) node)->agglevelsup > 0)
+ return node;
+ }
/*
* We should never see a SubPlan expression in the input (since this is
diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out
index 4c467c1b15..29e6b52a45 100644
--- a/src/test/regress/expected/groupingsets.out
+++ b/src/test/regress/expected/groupingsets.out
@@ -2040,4 +2040,49 @@ order by a, b, c;
| |
(11 rows)
+-- test handling of outer GroupingFunc within subqueries
+explain (costs off)
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
+ QUERY PLAN
+---------------------------
+ MixedAggregate
+ Hash Key: $2
+ Group Key: ()
+ InitPlan 1 (returns $1)
+ -> Result
+ InitPlan 3 (returns $2)
+ -> Result
+ -> Result
+ SubPlan 2
+ -> Result
+(10 rows)
+
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
+ grouping
+----------
+ 1
+ 0
+(2 rows)
+
+explain (costs off)
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
+ QUERY PLAN
+---------------------------
+ GroupAggregate
+ Group Key: $2
+ InitPlan 1 (returns $1)
+ -> Result
+ InitPlan 3 (returns $2)
+ -> Result
+ -> Result
+ SubPlan 2
+ -> Result
+(9 rows)
+
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
+ grouping
+----------
+ 0
+(1 row)
+
-- end
diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql
index 3944944704..c0786bbed8 100644
--- a/src/test/regress/sql/groupingsets.sql
+++ b/src/test/regress/sql/groupingsets.sql
@@ -555,4 +555,13 @@ from (values (1, 2, 3), (4, null, 6), (7, 8, 9)) as t (a, b, c)
group by rollup(a, b), rollup(a, c)
order by a, b, c;
+-- test handling of outer GroupingFunc within subqueries
+explain (costs off)
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
+
+explain (costs off)
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
+
-- end
--
2.31.0
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17088: FailedAssertion in prepagg.c
@ 2022-01-27 08:10 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Kyotaro Horiguchi @ 2022-01-27 08:10 UTC (permalink / raw)
To: guofenglinux@gmail.com; +Cc: tgl@sss.pgh.pa.us; andrew@tao11.riddles.org.uk; michael@paquier.xyz; cyg0810@gmail.com; pgsql-bugs@lists.postgresql.org
At Fri, 9 Jul 2021 14:54:02 +0800, Richard Guo <guofenglinux@gmail.com> wrote in
> Update the patch with comments and test cases.
AFAICS the patch looks correct. It works for the first example and
the two from Tom. I don't find other place that has the similar
issue.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: BUG #17088: FailedAssertion in prepagg.c
@ 2022-03-21 21:09 Tom Lane <tgl@sss.pgh.pa.us>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
0 siblings, 0 replies; 12+ messages in thread
From: Tom Lane @ 2022-03-21 21:09 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: guofenglinux@gmail.com; andrew@tao11.riddles.org.uk; michael@paquier.xyz; cyg0810@gmail.com; pgsql-bugs@lists.postgresql.org
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Fri, 9 Jul 2021 14:54:02 +0800, Richard Guo <guofenglinux@gmail.com> wrote in
>> Update the patch with comments and test cases.
> AFAICS the patch looks correct. It works for the first example and
> the two from Tom. I don't find other place that has the similar
> issue.
I'd been expecting Andrew to pick this up, but since he hasn't,
I took a look.
I concur that the core problem is that GroupingFunc has to be treated
almost exactly like Aggref, and here we have a couple of places that
didn't get that memo. So it occurred to me to look for other places
that special-case Aggref and don't have parallel code for GroupingFunc,
and I found several:
expression_returns_set_walker
This isn't particularly hazardous, since the argument (probably?) can't
contain a SRF, but it still seems like it ought to treat the two node
types the same.
cost_qual_eval_walker
It's defaulting to charging the eval costs of the arguments, which is
flat wrong. I made it charge one cpu_operator_cost instead.
ruleutils.c
Various places concerned with whether or not we need parens were
making the wrong choice, resulting in excess parens in pretty-printing
mode. This is also just cosmetic, but still.
This looks good to me now, and I'll set about back-patching.
regards, tom lane
Attachments:
[text/x-diff] v2-0001-fix-oversights-in-GroupingFunc-handling.patch (7.7K, ../../2319370.1647896994@sss.pgh.pa.us/2-v2-0001-fix-oversights-in-GroupingFunc-handling.patch)
download | inline diff:
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 47d0564fa2..ec25aae6e3 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -736,6 +736,8 @@ expression_returns_set_walker(Node *node, void *context)
/* Avoid recursion for some cases that parser checks not to return a set */
if (IsA(node, Aggref))
return false;
+ if (IsA(node, GroupingFunc))
+ return false;
if (IsA(node, WindowFunc))
return false;
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 8dc7dd4ca2..4d9f3b4bb6 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -4492,6 +4492,12 @@ cost_qual_eval_walker(Node *node, cost_qual_eval_context *context)
*/
return false; /* don't recurse into children */
}
+ else if (IsA(node, GroupingFunc))
+ {
+ /* Treat this as having cost 1 */
+ context->total.per_tuple += cpu_operator_cost;
+ return false; /* don't recurse into children */
+ }
else if (IsA(node, CoerceViaIO))
{
CoerceViaIO *iocoerce = (CoerceViaIO *) node;
diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c
index 41bd1ae7d4..863e0e24a1 100644
--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -356,15 +356,17 @@ build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot,
Node *arg = pitem->item;
/*
- * The Var, PlaceHolderVar, or Aggref has already been adjusted to
- * have the correct varlevelsup, phlevelsup, or agglevelsup.
+ * The Var, PlaceHolderVar, Aggref or GroupingFunc has already been
+ * adjusted to have the correct varlevelsup, phlevelsup, or
+ * agglevelsup.
*
- * If it's a PlaceHolderVar or Aggref, its arguments might contain
- * SubLinks, which have not yet been processed (see the comments for
- * SS_replace_correlation_vars). Do that now.
+ * If it's a PlaceHolderVar, Aggref or GroupingFunc, its arguments
+ * might contain SubLinks, which have not yet been processed (see the
+ * comments for SS_replace_correlation_vars). Do that now.
*/
if (IsA(arg, PlaceHolderVar) ||
- IsA(arg, Aggref))
+ IsA(arg, Aggref) ||
+ IsA(arg, GroupingFunc))
arg = SS_process_sublinks(root, arg, false);
splan->parParam = lappend_int(splan->parParam, pitem->paramId);
@@ -1957,10 +1959,11 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context)
}
/*
- * Don't recurse into the arguments of an outer PHV or aggregate here. Any
- * SubLinks in the arguments have to be dealt with at the outer query
- * level; they'll be handled when build_subplan collects the PHV or Aggref
- * into the arguments to be passed down to the current subplan.
+ * Don't recurse into the arguments of an outer PHV, Aggref or
+ * GroupingFunc here. Any SubLinks in the arguments have to be dealt with
+ * at the outer query level; they'll be handled when build_subplan
+ * collects the PHV, Aggref or GroupingFunc into the arguments to be
+ * passed down to the current subplan.
*/
if (IsA(node, PlaceHolderVar))
{
@@ -1972,6 +1975,11 @@ process_sublinks_mutator(Node *node, process_sublinks_context *context)
if (((Aggref *) node)->agglevelsup > 0)
return node;
}
+ else if (IsA(node, GroupingFunc))
+ {
+ if (((GroupingFunc *) node)->agglevelsup > 0)
+ return node;
+ }
/*
* We should never see a SubPlan expression in the input (since this is
@@ -2084,7 +2092,7 @@ SS_identify_outer_params(PlannerInfo *root)
outer_params = NULL;
for (proot = root->parent_root; proot != NULL; proot = proot->parent_root)
{
- /* Include ordinary Var/PHV/Aggref params */
+ /* Include ordinary Var/PHV/Aggref/GroupingFunc params */
foreach(l, proot->plan_params)
{
PlannerParamItem *pitem = (PlannerParamItem *) lfirst(l);
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index b16526e65e..7f4f3f7369 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -7956,12 +7956,13 @@ get_parameter(Param *param, deparse_context *context)
context->varprefix = true;
/*
- * A Param's expansion is typically a Var, Aggref, or upper-level
- * Param, which wouldn't need extra parentheses. Otherwise, insert
- * parens to ensure the expression looks atomic.
+ * A Param's expansion is typically a Var, Aggref, GroupingFunc, or
+ * upper-level Param, which wouldn't need extra parentheses.
+ * Otherwise, insert parens to ensure the expression looks atomic.
*/
need_paren = !(IsA(expr, Var) ||
IsA(expr, Aggref) ||
+ IsA(expr, GroupingFunc) ||
IsA(expr, Param));
if (need_paren)
appendStringInfoChar(context->buf, '(');
@@ -8089,6 +8090,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
case T_NextValueExpr:
case T_NullIfExpr:
case T_Aggref:
+ case T_GroupingFunc:
case T_WindowFunc:
case T_FuncExpr:
/* function-like: name(..) or name[..] */
@@ -8205,6 +8207,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
case T_XmlExpr: /* own parentheses */
case T_NullIfExpr: /* other separators */
case T_Aggref: /* own parentheses */
+ case T_GroupingFunc: /* own parentheses */
case T_WindowFunc: /* own parentheses */
case T_CaseExpr: /* other separators */
return true;
@@ -8255,6 +8258,7 @@ isSimpleNode(Node *node, Node *parentNode, int prettyFlags)
case T_XmlExpr: /* own parentheses */
case T_NullIfExpr: /* other separators */
case T_Aggref: /* own parentheses */
+ case T_GroupingFunc: /* own parentheses */
case T_WindowFunc: /* own parentheses */
case T_CaseExpr: /* other separators */
return true;
diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out
index 58a25b691a..6a56f0b09c 100644
--- a/src/test/regress/expected/groupingsets.out
+++ b/src/test/regress/expected/groupingsets.out
@@ -2042,4 +2042,49 @@ order by a, b, c;
| |
(11 rows)
+-- test handling of outer GroupingFunc within subqueries
+explain (costs off)
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
+ QUERY PLAN
+---------------------------
+ MixedAggregate
+ Hash Key: $2
+ Group Key: ()
+ InitPlan 1 (returns $1)
+ -> Result
+ InitPlan 3 (returns $2)
+ -> Result
+ -> Result
+ SubPlan 2
+ -> Result
+(10 rows)
+
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
+ grouping
+----------
+ 1
+ 0
+(2 rows)
+
+explain (costs off)
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
+ QUERY PLAN
+---------------------------
+ GroupAggregate
+ Group Key: $2
+ InitPlan 1 (returns $1)
+ -> Result
+ InitPlan 3 (returns $2)
+ -> Result
+ -> Result
+ SubPlan 2
+ -> Result
+(9 rows)
+
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
+ grouping
+----------
+ 0
+(1 row)
+
-- end
diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql
index 473d21f6b9..8050dbf260 100644
--- a/src/test/regress/sql/groupingsets.sql
+++ b/src/test/regress/sql/groupingsets.sql
@@ -557,4 +557,13 @@ from (values (1, 2, 3), (4, null, 6), (7, 8, 9)) as t (a, b, c)
group by rollup(a, b), rollup(a, c)
order by a, b, c;
+-- test handling of outer GroupingFunc within subqueries
+explain (costs off)
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by cube(v1);
+
+explain (costs off)
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
+select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1;
+
-- end
^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2022-03-21 21:09 UTC | newest]
Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-07-07 06:33 BUG #17088: FailedAssertion in prepagg.c PG Bug reporting form <noreply@postgresql.org>
2021-07-07 11:00 ` Michael Paquier <michael@paquier.xyz>
2021-07-07 20:49 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-07-07 21:18 ` Andrew Gierth <andrew@tao11.riddles.org.uk>
2021-07-07 21:22 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-07-07 21:32 ` Andrew Gierth <andrew@tao11.riddles.org.uk>
2021-07-07 21:56 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-07-08 05:44 ` Richard Guo <guofenglinux@gmail.com>
2021-07-08 06:14 ` Richard Guo <guofenglinux@gmail.com>
2021-07-09 06:54 ` Richard Guo <guofenglinux@gmail.com>
2022-01-27 08:10 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2022-03-21 21:09 ` Tom Lane <tgl@sss.pgh.pa.us>
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