Re: BUG #17088: FailedAssertion in prepagg.c

Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> writes:
> At Fri, 9 Jul 2021 14:54:02 +0800, Richard Guo <guofenglinux(at)gmail(dot)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