Re: Aggregate function API versus grouping sets

I've been assisting Atri with development of an implementation of
GROUPING SETS, beginning with a one-pass implementation of ROLLUP.
Two issues have arisen regarding the API for calling aggregate
transition and final functions that I think need answering now,
since they relate to changes in 9.4.

1. Assumptions about the relationship between aggcontext and fn_extra

Tom's version of the ordered-set aggregate code makes the assumption
that it is safe to store the aggcontext returned by AggCheckCallContext
in a structure hung off flinfo->fn_extra. This implicitly assumes that
the function will be called in only one aggcontext, or at least only one
per flinfo.

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?

2. AggGetPerAggEContext

The comment documents this as returning the per-output-tuple econtext,
and the ordered-set code assumes that the rescan of this context implies
that the aggcontext is also about to be reset (so cleanup functions can
be called).

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? (In fact what I'm
considering is making aggcontext be the per-tuple memory context of an
ExprContext created for each grouping set, and having
AggGetPerAggEContext return this - but it won't be the actual context in
which the result projection happens.)

--
Andrew (irc:RhodiumToad)

Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> writes:
> 1. Assumptions about the relationship between aggcontext and fn_extra

> Tom's version of the ordered-set aggregate code makes the assumption
> that it is safe to store the aggcontext returned by AggCheckCallContext
> in a structure hung off flinfo->fn_extra. This implicitly assumes that
> the function will be called in only one aggcontext, or at least only one
> per flinfo.

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

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

> 2. AggGetPerAggEContext

> The comment documents this as returning the per-output-tuple econtext,
> and the ordered-set code assumes that the rescan of this context implies
> that the aggcontext is also about to be reset (so cleanup functions can
> be called).

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

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

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

Obviously, the time grows short to reconsider this API, but it's not
quite too late.

regards, tom lane

>>>>> "Tom" == Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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)

Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> writes:
> "Tom" == Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> 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.

If we're going to do that, I think it needs to be in 9.4. Are you
going to work up a patch?

regards, tom lane

>>>>> "Tom" == Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

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.

Tom> If we're going to do that, I think it needs to be in 9.4. Are
Tom> you going to work up a patch?

Do we want a decision on the fn_extra matter first, or shall I do one
patch for the econtext, and a following one for fn_extra?

--
Andrew (irc:RhodiumToad)

Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> writes:
> "Tom" == Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Tom> If we're going to do that, I think it needs to be in 9.4. Are
> Tom> you going to work up a patch?

> Do we want a decision on the fn_extra matter first, or shall I do one
> patch for the econtext, and a following one for fn_extra?

I think they're somewhat independent, and probably best patched
separately. In any case orderedsetagg.c's use of fn_extra is a local
matter that we'd not really have to fix in 9.4, except to the extent
that you think third-party code might copy it.

regards, tom lane

>>>>> "Tom" == Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

>> Do we want a decision on the fn_extra matter first, or shall I do
>> one patch for the econtext, and a following one for fn_extra?

Tom> I think they're somewhat independent, and probably best patched
Tom> separately. In any case orderedsetagg.c's use of fn_extra is a
Tom> local matter that we'd not really have to fix in 9.4, except to
Tom> the extent that you think third-party code might copy it.

Given that there's been no attempt to expose ordered_set_startup /
ordered_set_transition* as some sort of API, I think it's virtually
inevitable that people will cargo-cult all of that code into any new
ordered set aggregate they might wish to create.

(Had one request so far for a mode() variant that returns the unique
modal value if one exists, otherwise null; so the current set of
ordered-set aggs by no means exhausts the possible applications.)

--
Andrew (irc:RhodiumToad)

>>>>> "Tom" == Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

Tom> If we're going to do that, I think it needs to be in 9.4. Are
Tom> you going to work up a patch?

How's this? (applies and passes regression on 9.4 and master)

--
Andrew (irc:RhodiumToad)

Here are two more, cumulative with the previous patch and each other:

The first removes the assumption in ordered_set_startup that the
aggcontext can be cached in fn_extra, and puts it in the transvalue
instead.

The second exposes the OSA* structures in a header file, so that
user-defined ordered-set aggs can use ordered_set_transition[_multi]
directly rather than having to cargo-cult it into their own code.

--
Andrew (irc:RhodiumToad)

Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> writes:
> "Tom" == Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Tom> If we're going to do that, I think it needs to be in 9.4. Are
> Tom> you going to work up a patch?

> How's this? (applies and passes regression on 9.4 and master)

Pushed with minor cosmetic adjustments.

regards, tom lane

>>>>> "Tom" == Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

>> How's this? (applies and passes regression on 9.4 and master)

Tom> Pushed with minor cosmetic adjustments.

Thanks!

--
Andrew (irc:RhodiumToad)

Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk> writes:
> Here are two more, cumulative with the previous patch and each other:
> The first removes the assumption in ordered_set_startup that the
> aggcontext can be cached in fn_extra, and puts it in the transvalue
> instead.

OK, done. (ATM it seems like we wouldn't need gcontext in the transvalue
either, but I left it there in case we want it later.)

> The second exposes the OSA* structures in a header file, so that
> user-defined ordered-set aggs can use ordered_set_transition[_multi]
> directly rather than having to cargo-cult it into their own code.

I'm not very happy about this one; we intentionally did not expose
these structs, so that we could freely make fixes like, for example,
your immediately preceding patch.

I think it'd be worth considering whether there's a way to allow
third-party ordered-set aggs to use the infrastructure in orderedsetaggs.c
while still treating these structs as opaque. In any case we'd need a
more carefully thought-through API than just decreeing that some private
structs are now public.

regards, tom lane

On 07/02/2014 10:15 PM, Andrew Gierth wrote:
> (Had one request so far for a mode() variant that returns the unique
> modal value if one exists, otherwise null; so the current set of
> ordered-set aggs by no means exhausts the possible applications.)

I resemble that remark. :)

But seriously, am I the only one who doesn't want some random value when
there is no dominant one? And the fact that I can't create my own
without C code is a real bummer.
--
Vik