Re: Sharing aggregate states between different aggregate functions

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:
>
>> - * agg_input_types, agg_state_type, agg_result_type identify the input,
>> - * transition, and result types of the aggregate. These should all be
>> - * resolved to actual types (ie, none should ever be ANYELEMENT etc).
>> + * agg_input_types identifies the input types of the aggregate. These
>> should
>> + * be resolved to actual types (ie, none should ever be ANYELEMENT etc).
>>
>> I'm not sure I understand why agg_state_type and agg_result_type have
>> changed here.
>>
>
> The function no longer has the agg_result_type argument, but the removal
> of agg_state_type from the comment was a mistake.

I've put agg_state_type back in the attached delta which is again based on
your version of the patch.

>
>
> + peraggstate->sortstates = (Tuplesortstate **)
>> + palloc0(sizeof(Tuplesortstate *) * numGroupingSets);
>> + for (currentsortno = 0; currentsortno < numGroupingSets;
>> currentsortno++)
>> + peraggstate->sortstates[currentsortno] = NULL;
>>
>> This was not you, but this NULL setting looks unneeded due to the
>> palloc0().
>>
>
> Yeah, I noticed that too. Ok, let's take it out.
>
>
Removed in attached.

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

In summary, I'm much less confident it's safe to enable the optimisation in
this case.

> BTW, the name of the AggStatePerAggStateData struct is pretty horrible.
>>> The repeated "AggState" feels awkward. Now that I've stared at the patch
>>> for a some time, it doesn't bother me anymore, but it took me quite a
>>> while
>>> to over that. I'm sure it will for others too. And it's not just that
>>> struct, the comments talk about "aggregate state", which could be
>>> confused
>>> to mean "AggState", but it actually means AggStatePerAggStateData. I
>>> don't
>>> have any great suggestions, but can you come up a better naming scheme?
>>>
>>
>> I agree, they're horrible. The thing that's causing the biggest problem is
>> the struct named AggState, which carries state for *all* aggregates, and
>> has nothing to do with "transition state", so it seems there's two
>> different meanings if the word "state" and I've used both meanings in the
>> name for AggStatePerAggStateData.
>>
>> Perhaps just renaming AggStatePerAggStateData to AggStateTransStateData
>> would be good enough?
>>
>
> Hmm. I think it should be "AggStatePerTransData" then, to keep the same
> pattern as AggStatePerAggData and AggStatePerGroupData.
>
>
Sounds good. I've renamed it to that in the attached delta patch.

Regards

David Rowley

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