Re: Combining Aggregates

On Thu, Mar 24, 2016 at 1:17 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I'm going to read through the code again now.

OK, I noticed another documentation problem: you need to update
catalogs.sgml for these new columns.

+ * Validate the serial function, if present. We must ensure
that the return
+ * Validate the de-serial function, if present. We must ensure that the

I think that you should refer to these consistently in the comments as
the "serialization function" and the "deserialization function", even
though the SQL syntax is different. And unhyphenated.

+ /* check that we also got a serial type */
+ if (!OidIsValid(aggSerialType))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("must specify serialization
type when specifying serialization function")));

I think that in parallel cases, this check is in DefineAggregate(),
not here. See, e.g. "aggregate mfinalfunc must not be specified
without mstype".

Existing type parameters to CREATE AGGREGATE have IsPolymorphicType()
checks to enforce sanity in various ways, but you seem not to have
added that for the serial type.

+ /* don't call a strict serial function with
NULL input */
+ if (pertrans->serialfn.fn_strict &&
+ pergroupstate->transValueIsNull)
+ continue;

Shouldn't this instead set aggnulls[aggno] = true? And doesn't the
hunk in combine_aggregates() have the same problem?

+ /*
+ * serial and de-serial functions must match, if
present. Remember that
+ * these will be InvalidOid if they're not required
for this agg node
+ */

Explain WHY they need to match. And maybe update the overall comment
for the function.

+ "'-' AS
aggdeserialfn,aggmtransfn, aggminvtransfn, "

Whitespace.

In your pg_aggregate.h changes, avg(numeric) sets aggserialtype but no
aggserialfn or aggdeserialfn.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company