Horton

Re: Combination of ordered-set aggregate function terminates JDBC connection on PostgreSQL 9.6.5

Heikki Linnakangas <hlinnaka@iki.fi>

View thread
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Tom Lane <tgl@sss.pgh.pa.us>, David Rowley <david.rowley@2ndquadrant.com>
Cc: Lukas Eder <lukas.eder@gmail.com>, PostgreSQL mailing lists <pgsql-bugs@postgresql.org>
Date: 2017-10-12T13:59:01Z
Lists: pgsql-bugs

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Treat aggregate direct arguments as per-agg data not per-trans data.

  2. Allow the built-in ordered-set aggregates to share transition state.

  3. Fix AggGetAggref() so it won't lie to aggregate final functions.

  4. Prevent sharing transition states between ordered-set aggregates.

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