Re: [HACKERS] Aggregates with context - a question

Philip Warner <pjw@rhyme.com.au> writes:
> The problems is that the way I read the code, there is an expectation
> that the parameters of sfuncX and finalfunc match the basetype.

No, not at all.  transfn1 has the signaturetranstype1 = transfn1 (transtype1, basetype)
ie, you take the state object and the next input value, and deliver a
new state object.  transfn2 has the signaturetranstype2 = transfn2 (transtype2)
ie, it operates on the second state object and delivers a new value.
And finalfn has the signaturefinaltype = finalfn (transtype1, transtype2)
ie, take the two state objects and deliver the desired result.
(You can also use a finalfn that takes only one state object,
if you are only using one transfn.)

If you omit finalfn then you can only supply one transfn, and the
corresponding transtype must equal finaltype because the default
behavior is just to copy the state value computed by the transfn
that's present.  But as long as you have a finalfn, all four datatypes
can be different.

This undoubtedly seems overly baroque, and it is overkill when writing
a specialized transfn from scratch; there's no need for transfn2, just
a transfn1 and a finalfn to produce the end result.  The reason it was
done this way (I assume; I wasn't there) is that you can produce many
useful aggregates by combining *existing* operators without having to
write any code at all.  For example, avg() for float8 is built from
transfn1 = float8pl, transfn2 = float8inc, and finalfn = float8div.

I am not sure whether this is adequately documented; probably not.
Will revisit the docs for 6.6 (I suppose Thomas will have my head
if I change 'em now ;-)).

> This is clearly not the case, and I have declared them as int4, which
> will also presumable break on 64 bit implementations...

The "clean" way to do it is to make a declared datatype that corresponds
to the state storage you need, but that's overkill if such a datatype
hasn't got any other use.  I think the way you have done it is a
reasonable cheat, though I agree that using int4 is risky for
portability.  There has been some talk of inventing a type OID
representing "C string", and that (when available) might be a better way
of declaring transtype1 when it's really a private struct of some sort.

One thing you have to be very careful about is memory allocation and
lifetime.  The way you are doing it, a palloc in the first transfn1
iteration and a pfree in finalfn, will be fine.  However this may change
in 6.6, since we are going to have to do something to cure memory leaks
in aggregation.  (Currently, if the transfns are ones that palloc their
result value, as all float8 ops do for example, the storage is not
reclaimed until end of transaction.  That's no good if there are lots of
tuples...)

> Anyway, the code is below.

Looks OK except you are potentially pfreeing an uninit pointer in the
finalfn...
        regards, tom lane