Sharing aggregate states between different aggregate functions

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.

Regards

David Rowley

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

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 27 July 2015 at 03:24, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

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?

Thanks for taking the time to look at this and makes these fixes.

I'm just looking over your changes:

- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("aggregate %u needs to have compatible input type and transition
type",
- aggref->aggfnoid)));
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("aggregate needs to have compatible input type and transition
type")));

I can't quite see the reason to remove the agg OID from the error message
here. It seems to be still valid to use as build_peraggstate_for_aggref()
only is called when nothing is shared.

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

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

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.

Good catch, and accurate analogy. Thanks for fixing.

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

Good idea. I failed to notice that code over there in execQual.c so I agree
that where you've moved it to is much better.

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

Thanks for fixing.

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?

select aggfnoid || '(' || typname || ')',aggtransfn,agginitval
from pg_aggregate
inner join pg_type on aggtranstype = oid
where aggtransfn in (select aggtransfn
from pg_aggregate
group by aggtransfn
having count(*)>1)
order by aggtransfn;

This indicates that everything using float4_accum as a transfn could
benefit from that. I just wasn't sure how far to go.

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

That's much better.

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

I don't think I'm going to get the time to work on the "supporting
aggregate" stuff you're talking about, but I think it's a good enough idea
to keep around for the future, so I think this shared aggregate states
stuff probably should go into nodeAgg.c for now. I have to say though, I
was a little surprised to find this code in the executor rather than the
planner when I first started on this.

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?

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?

Regards

David Rowley

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 27 July 2015 at 18:15, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:

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.

Thanks for testing this.

You should only see an improvement on aggregates listed here:

select aggfnoid::oid, aggfnoid || '(' || typname ||
')',aggtransfn,agginitval
from pg_aggregate ag
inner join pg_proc pr on aggfnoid = pr.oid
inner join pg_type tp on pr.proargtypes[0] = tp.oid
where ag.aggtransfn in (select aggtransfn
from pg_aggregate
group by aggtransfn
having count(*)>1)
and ag.agginitval is null
order by ag.aggtransfn;

Regards

David Rowley

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

Yeah, it's awkward, as I just managed to remind myself:

The aggtranstype needs to be known before we can call GetAggInitVal() on
the initval datum.

That currently happens in build_transstate_for_aggref() only when we've
decided to create a new state.

transstate->initValue = GetAggInitVal(textInitVal,
transstate->aggtranstype);

And to get the aggtranstype:

transstate->aggtranstype =
resolve_aggregate_transtype(aggref->aggfnoid,
aggform->aggtranstype,
inputTypes,
numArguments);

Of course, not impossible, but lots of code gets duplicated.

Regards

David Rowley

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

On 27 July 2015 at 20:11, Heikki Linnakangas <hlinnaka@iki.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/&gt;
PostgreSQL Development, 24x7 Support, Training & Services

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 29 July 2015 at 03:45, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

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.

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/&gt;
PostgreSQL Development, 24x7 Support, Training & Services

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 5 August 2015 at 03:03, Heikki Linnakangas <hlinnaka@iki.fi> wrote:

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!

Great! Thanks for doing the cleanups and committing it.

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