>>>>> "Tom" == Tom Lane <t...@sss.pgh.pa.us> writes:

 >> Doing rollup via GroupAggregate by maintaining multiple transition
 >> values at a time (one per grouping set) means that the transfn is
 >> being called interleaved for transition values in different
 >> contexts. So the question becomes: is it wrong for the transition
 >> function to assume that aggcontext can be cached this way, or is
 >> it necessary for the executor to use a separate flinfo for each
 >> concurrent grouping set?

 Tom> Hm.  We could probably move gcontext into the per-group data.
 Tom> I'm not sure though if there are any other dependencies there on
 Tom> the groups being evaluated serially.  More generally, I wonder
 Tom> whether user-defined aggregates are likely to be making
 Tom> assumptions that will be broken by this.

The thing is that almost everything _except_ aggcontext and
AggGetPerAggEContext that a transfn might want to hang off fn_extra
really is going to be constant across all groups.

The big question, as you say, is whether this is going to be an issue
for existing user-defined aggregates.

 >> Since it seems that the cleanup callback is the sole reason for
 >> this function to exist, is it acceptable to remove any implication
 >> that the context returned is the overall per-output-tuple one, and
 >> simply state that it is a context whose cleanup functions are
 >> called at the appropriate time before aggcontext is reset?

 Tom> Well, I think the implication is that it's the econtext in which
 Tom> the result of the aggregation will be used.

In the rollup case, though, it does not seem reasonable to have
multiple result-tuple econtexts (that would significantly complicate
the projection of rows, the handling of rescans, etc.).

 Tom> Another approach would be to remove AggGetPerAggEContext as such
 Tom> from the API altogether, and instead offer an interface that
 Tom> says "register an aggregate cleanup callback", leaving it to the
 Tom> agg/window core code to figure out which context to hang that
 Tom> on.  I had thought that exposing this econtext and the
 Tom> per-input-tuple one would provide useful generality, but maybe
 Tom> we should rethink that.

Works for me.

Andrew (irc:RhodiumToad)

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

Reply via email to