Re: [PATCH] Negative Transition Aggregate Functions (WIP)

On Apr9, 2014, at 20:20 , Tom Lane <tgl@sss.pgh.pa.us> wrote:
> There was discussion upthread of providing
> two separate forward transition functions, but Florian argued that that
> would do nothing that you couldn't accomplish with a runtime check in
> the forward function.  I think that's nonsense though, because one of
> the key points here is that an invertible aggregate may require a more
> complex transition state data structure --- in particular, if you're
> forced to go from a pass-by-value to a pass-by-reference data type, right
> there you are going to take a big hit in aggregate performance, and there
> is no way for the forward transition function to avoid it.

To be precise, my point was that *only* having a separate non-invertible
forward transition function is pointless, exactly because of the reason
you gave - that won't allow a cheaper state representation for the
non-invertible case. So all such a non-invertible forward transition
function can do is to skip some bookkeeping that's required by the
inverse transition function - and *that* can just as easily be accomplished
by a runtime check.

I was (and still am) not in favour of duplicating the whole quadruple of
(state, initialvalue, transferfunction, finalfunction) because it seems
excessive. In fact, I believed that doing this would probably be grounds for
outright rejection of the patch, on the base of catalog bloat. And your
initial response to this suggestion seemed to confirm this.

> The patch has in fact already done that to a couple of basic aggregates like
> sum(int4).  Has anyone bothered to test what side-effects that has on
> non-windowed aggregation performance?

I'm pretty sure David Rowley did some benchmarking. The results should be
in this thread somewhere I think, but they currently evade me... Maybe David
can re-post, if he's following this...

> I think what'd make sense is to have a separate forward function *and*
> separate state datatype to be used when we want invertible aggregation
> (hm, and I guess a separate final function too).  So an aggregate
> definition would include two entirely independent implementations.
> If they chance to share sfunc and stype, fine, but they don't have to.
>
> This would mean we'd need some new names for the doppelgangers of the
> CREATE AGGREGATE parameters sfunc, stype, sspace, finalfunc, and initcond
> (but not sortop).  I guess that'd open up a chance to use a more uniform
> naming scheme, but I'm not too sure what would be good.

If we really go down that road (and I'm far from convinced), then maybe
instead of having a bunch of additional fields, we could have separate
entries in pg_aggregate for the two cases, with links between them.

The non-invertible aggregates would have something like "_noninv" or so
appended to their name, and we'd automatically use them if we know we
won't need to remove entries from the aggregation state. That would also
allow the users to *force* the non-invertible aggregate to be used by
simply saying "SUM_NONINV" instead of "SUM".

Then all we'd need would be an additional OID field that links the
invertible to the non-invertible definition.

best regards,
Florian Pflug