On Thu, Mar 24, 2016 at 1:17 PM, Robert Haas <robertmh...@gmail.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 -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers