Re: Sharing aggregate states between different aggregate functions - Mailing list pgsql-hackers

On 07/28/2015 04:14 AM, David Rowley wrote:

On 27 July 2015 at 20:11, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

On 07/27/2015 08:34 AM, David Rowley wrote:

In this function I also wasn't quite sure if it was with comparing both
non-NULL INITCOND's here. I believe my code comments may slightly
contradict what the code actually does, as the comments talk about them
having to match, but the code just bails if any are non-NULL. The reason I
didn't check them was because it seems inevitable that some duplicate work
needs to be done when setting up the INITCOND. Perhaps it's worth it?

It would be nice to handle non-NULL initconds. I think you'll have to
check that the input function isn't volatile. Or perhaps just call the
input function, and check that the resulting Datum is byte-per-byte
identical, although that might be awkward to do with the current code
structure.

I've not done anything with this.
I'd not thought of an input function being volatile before, but I guess
it's possible, which makes me a bit scared that we could be treading on
ground we shouldn't be. I know it's more of an output function thing than
an input function thing, but a GUC like extra_float_digits could cause
problems here.

Yeah, a volatile input function seems highly unlikely, but who knows. BTW, we're also not checking if the transition or final functions are volatile. But that was the same before this patch too.

It sure would be nice to support the built-in float aggregates, so I took a stab at this. I heavily restructured the code again, so that there are now two separate steps. First, we check for any identical Aggrefs that could be shared. If that fails, we proceed to the permission checks, look up the transition function and build the initial datum. And then we call another function that tries to find an existing, compatible per-trans structure. I think this actually looks better than before, and checking for identical init values is now easy. This does lose one optimization: if there are two aggregates with identical transition functions and final functions, they are not merged into a single per-Agg struct. They still share the same per-Trans struct, though, and I think that's enough.

How does the attached patch look to you? The comments still need some cleanup, in particular, the explanations of the different scenarios don't belong where they are anymore.

BTW, the permission checks were not correct before. You cannot skip the check on the transition function when you're sharing the per-trans state. We check that the aggregate's owner has permission to execute the transition function, and the previous aggregate whose state value we're sharing might have different owner.