BUG #17088: FailedAssertion in prepagg.c

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

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

"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)

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

"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.

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

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

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

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

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

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