On 10/12/2017 05:27 AM, Tom Lane wrote:
>>> This might be a bigger change than we want to push into the back
>>> branches. In that case probably a back-patchable fix is to hw ck
>>> nodeAgg.c so it will never combine input states for OSAs.
>
>> I've attached a patch which does this.
>
> Needs to reject plain OSAs too, not just hypotheticals. Pushed
> with that fix and some test cases.
Thanks!
> Speaking of AggGetAggref, there's another thing that I think 804163bc2
> did wrong for ordered-set aggregates: it can return the wrong Aggref
> when two aggregates' intermediate states have been merged. I do not
> think it's appropriate to say "well, you shouldn't care which of the
> Aggrefs you get". It looks like this accidentally fails to fail
> for the current OSAs, because indeed they do only look at the input-
> related fields of the Aggref, but surely that's not something to
> rely on. It's most certainly not acceptable that the function's
> documentation doesn't mention that its result may be a lie.
Hmm. All the fields except for aggfnoid, aggtype and aggcollid are
related to the inputs or the transition function, so all the other
fields would be the same between two shared transition states. But yes,
this really should be documented. Perhaps AggGetAggref() should return
an Aggref with those fields set to InvalidOid, to make it clear that
they should not be looked at?
Conceivably we could have another function like AggGetAggref() that
returns all of the Aggrefs. But I don't think it's worth the
complication. If the transition function needs to do something different
depending on the aggregate it's for, well, don't do that. Define a
different transition function for both aggregates.
- Heikki
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs