Re: Sharing aggregate states between different aggregate functions

On 29 July 2015 at 03:45, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:

> On 07/28/2015 04:14 AM, David Rowley wrote:
>
>> On 27 July 2015 at 20:11, Heikki Linnakangas <hlinnaka(at)iki(dot)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.
>

I've read over the patch and you've managed to implement the init value
checking much more cleanly than I had imagined it to be.
I like the 2 stage checking.

Attached is a delta patched which is based
on sharing_aggstate-heikki-2.patch to fix up the out-dated comments and
also a few more test scenarios which test the sharing works with matching
INITCOND and that it does not when they don't match.

What do you think?

>
> 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.

oops, thank for noticing that and fixing.

Regards

David Rowley

--
David Rowley http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Training & Services