Hi,
David, anyone, any comments?
On 2019-05-16 20:04:04 -0700, Andres Freund wrote:
> On 2019-05-08 13:06:36 +0900, Kyotaro HORIGUCHI wrote:
> > This behavior is introduced by 69c3936a14 (in v11). At that time
> > FunctionCallInfoData is pallioc0'ed and has fixed length members
> > arg[6] and argnull[7]. So nulls[1] is always false even if nargs
> > = 1 so the issue had not been revealed.
>
> > After introducing a9c35cf85c (in v12) the same check is done on
> > FunctionCallInfoData that has NullableDatum args[] of required
> > number of elements. In that case args[1] is out of palloc'ed
> > memory so this issue has been revealed.
>
> > 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.
>
> It's documentation says:
>
> /*
> * Number of aggregated input columns to pass to the transfn. This
> * includes the ORDER BY columns for ordered-set aggs, but not for plain
> * aggs. (This doesn't count the transition state value!)
> */
> int numTransInputs;
>
> which IMO is violated by having it set to the plain aggregate's value,
> rather than the combine func.
>
> 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.
>
> It seems pretty clear that this needs to be fixed for v11, it seems too
> fragile to rely on trans_fcinfo->argnull[2] being zero initialized.
>
> I'm less sure about fixing it for 9.6/10. There's no use of
> numTransInputs for combining back then.
>
> David, I assume you didn't adjust numTransInput plainly because it
> wasn't needed / you didn't notice? Do you have a preference for a fix?
Unless somebody comments I'm later today going to move the numTransInput
computation into the DO_AGGSPLIT_COMBINE branch in
build_pertrans_for_aggref(), add a small test (using
enable_partitionwise_aggregate), and backpatch to 11.
Greetings,
Andres Freund
