On Fri, 17 May 2019 at 15:04, Andres Freund <andres@anarazel.de> wrote:
>
> On 2019-05-08 13:06:36 +0900, Kyotaro HORIGUCHI wrote:
> > In a second look, I seems to me that the right thing to do here
> > is setting numInputs instaed of numArguments to numTransInputs in
> > combining step.
>
> Yea, to me this just seems a consequence of the wrong
> numTransInputs. Arguably this is a bug going back to 9.6, where
> combining aggregates where introduced. It's just that numTransInputs
> isn't used anywhere for combining aggregates, before 11.
Isn't it more due to the lack of any aggregates with > 1 arg having a
combine function?
> While I agree that fixing numTransInputs is the right way, I'm not
> convinced the way you did it is the right approach. I'm somewhat
> inclined to think that it's wrong that ExecInitAgg() calls
> build_pertrans_for_aggref() with a numArguments that's not actually
> right? Alternatively I think we should just move the numTransInputs
> computation into the existing branch around DO_AGGSPLIT_COMBINE.
Yeah, probably we should be passing in the correct arg count for the
combinefn to build_pertrans_for_aggref(). However, I see that we also
pass in the inputTypes from the transfn, just we don't use them when
working with the combinefn.
You'll notice that I've just hardcoded the numTransArgs to set it to 1
when we're working with a combinefn. The combinefn always requires 2
args of trans type, so this seems pretty valid to me. I think
Kyotaro's patch setting of numInputs is wrong. It just happens to
accidentally match. I also added a regression test to exercise
regr_count. I tagged it onto an existing query so as to minimise the
overhead. It seems worth doing since most other aggs have a single
argument and this one wasn't working because it had two args.
I also noticed that the code seemed to work in af025eed536d, so I
guess the new expression evaluation code is highlighting the existing
issue.
> I feel this code has become quite creaky in the last few years.
You're not kidding!
Patch attached of how I think we should fix it.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services