Thread: Sharing aggregate states between different aggregate functions

On 07/09/2015 12:44 PM, David Rowley wrote:
> On 15 June 2015 at 12:05, David Rowley <david.rowley@2ndquadrant.com> wrote:
>
>>
>> This basically allows an aggregate's state to be shared between other
>> aggregate functions when both aggregate's transition functions (and a few
>> other things) match
>> There's quite a number of aggregates in our standard set which will
>> benefit from this optimisation.
>>
> After compiling the original patch with another compiler, I noticed a
> couple of warnings.
>
> The attached fixes these.

I spent some time reviewing this. I refactored the ExecInitAgg code
rather heavily to make it more readable (IMHO); see attached. What do
you think? Did I break anything?

Some comments:

* In aggref_has_compatible_states(), you give up if aggtype or aggcollid
differ. But those properties apply to the final function, so you were
leaving some money on the table by disallowing state-sharing if they differ.

* The filter and input expressions are initialized for every AggRef,
before the deduplication logic kicks in. The AggrefExprState.aggfilter,
aggdirectargs and args fields really belong to the AggStatePerAggState
struct instead. This is not a new issue, but now that we have a
convenient per-aggstate struct to put them in, let's do so.

* There was a reference-after free bug in aggref_has_compatible_states;
you cannot ReleaseSysCache and then continue pointing to the struct.

* The code was a bit fuzzy on which parts of the per-aggstate are filled
in at what time. Some of the fields were overwritten every time a match
was found. With the same values, so no harm done, but I found it
confusing. I refactored ExecInitAgg in the attached patch to clear that up.

* There API of build_aggregate_fnexprs() was a bit strange now that some
callers use it to only fill in the final function, some call it to fill
both the transition functions and the final function. I split it to two
separate functions.

* I wonder if we should do this duplicate elimination at plan time. It's
very fast, so I'm not worried about that right now, but you had grand
plans to expand this so that an aggregate could optionally use one of
many different kinds of state values. At that point, it certainly seems
like a planning decision to decide which aggregates share state. I think
we can leave it as it is for now, but it's something to perhaps consider
later.


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?

- Heikki

On Thu, Jul 9, 2015 at 7:44 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
> On 15 June 2015 at 12:05, David Rowley <david.rowley@2ndquadrant.com> wrote:
>>
>>
>> This basically allows an aggregate's state to be shared between other
>> aggregate functions when both aggregate's transition functions (and a few
>> other things) match
>> There's quite a number of aggregates in our standard set which will
>> benefit from this optimisation.
>>
>
> After compiling the original patch with another compiler, I noticed a couple
> of warnings.
>
> The attached fixes these.

I did some performance tests on the patch. This patch shown good
improvement for same column aggregates. With int or bigint datatype columns,
this patch doesn't show any visible performance difference. But with numeric
datatype it shows good improvement.

select sum(x), avg(y) from test where x < $1;

Different columns:

selectivity            Head            patch
(millions)
0.1                       315              322
0.3                       367              376
0.5                       419              427
1                          551              558
2                          824              826

select sum(x), avg(x) from test where x < $1;

Same column:

selectivity            Head            patch
(millions)
0.1                       314              314
0.3                       363              343
0.5                       412              373
1                          536              440
2                          795              586

Regards,
Hari Babu
Fujitsu Australia

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.

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

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

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

> I've attached a delta patch based on your patch, in this I've:
>
> 1. Renamed AggStatePerAggStateData to AggStateTransStateData and all
> variables using that are renamed to suit better.
> 2. Removed surplus peraggstate->sortstates[currentsortno] = NULL; (not
> related to this patch, but since we're moving that part of the code, we'd
> better fix)
> 3. Put back the missing aggfnoid from the error message.
> 4. Changed initialize_aggregates() to not pass the states. They're already
> in AggState and we're using aggstate->numstates to get the count of the
> items in that array, so it seems wrong to allow a different array to ever
> be passed in.
> 5. Changed wording of a few comments to try and reduce confusing of 'state'
> and 'transition state'.
> 6. Renamed AggState.peraggstate to transstates. I pluralised this to try to
> reduce confusion of the single state pointers named 'transstate' in the
> functions in nodeAgg.c. I did think that peragg should also become peraggs
> and pergroup should become pergroups, but didn't change those.
>
> Anything else I changed is self explanatory.
>
> What do you think?

Looks good, thanks!

- Heikki

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.

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

Thanks!

- Heikki

Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 07/28/2015 04:14 AM, David Rowley wrote:
>> 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.

GUC dependence is considered to make a function stable not volatile.
(I realize you can probably break that if you try hard enough, but
then you get to keep both pieces.)

> Yeah, a volatile input function seems highly unlikely, but who knows. 

We have a project policy against volatile I/O functions.  One reason why
is that it would break the assumption that record_in/record_out can be
marked stable.  I think there are other reasons too.

> BTW, we're also not checking if the transition or final functions are 
> volatile. But that was the same before this patch too.

Up to now it hasn't mattered.  Possibly this patch should refuse to
combine states across volatile transition functions?
        regards, tom lane

On 07/28/2015 07:18 PM, Tom Lane wrote:
> Heikki Linnakangas <hlinnaka@iki.fi> writes:
>> On 07/28/2015 04:14 AM, David Rowley wrote:
>> Yeah, a volatile input function seems highly unlikely, but who knows.
>
> We have a project policy against volatile I/O functions.  One reason why
> is that it would break the assumption that record_in/record_out can be
> marked stable.  I think there are other reasons too.

Ok. In the latest patch I'm not relying that anyway, so it doesn't 
matter, but good to know.

>> BTW, we're also not checking if the transition or final functions are
>> volatile. But that was the same before this patch too.
>
> Up to now it hasn't mattered.

Yes, it has. We combine identical aggregates even without this patch. 
For example:

SELECT sum(x), sum(x) FROM foo

Sum(x) gets calculated only once. If its transition function or final 
function was volatile, that could produce two different results if we 
ran the aggregate twice.

No-one's complained so far, and I can't think of a use case for a 
volatile transition or final function, so maybe it's not worth worrying 
about. Then again, checking for the volatility of those functions would 
be easy too.

- Heikki

Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 07/28/2015 07:18 PM, Tom Lane wrote:
>> Heikki Linnakangas <hlinnaka@iki.fi> writes:
>>> BTW, we're also not checking if the transition or final functions are
>>> volatile. But that was the same before this patch too.

>> Up to now it hasn't mattered.

> Yes, it has. We combine identical aggregates even without this patch. 

Ah, right, how'd I forget about that?

> No-one's complained so far, and I can't think of a use case for a 
> volatile transition or final function, so maybe it's not worth worrying 
> about. Then again, checking for the volatility of those functions would 
> be easy too.

Given the lack of complaints, I tend to agree that it's not the province
of this patch to make a change in that policy.
        regards, tom lane

On 08/03/2015 08:53 AM, David Rowley wrote:
> 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?

I committed this, after some more cleanup of the comments. Thanks!

- Heikki