Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> OK, I'm marking this ready for committer attention, on the
> understanding that that doesn't include the invtrans_minmax patch.
I've started to look at this patch set. After rereading the thread,
I'm thinking that it's a mistake to just add the inverse transition
function and require it to work with the standard forward transition
function for the aggregate. 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. 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 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.
Comments?
regards, tom lane