The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation: not tested
This applies (with some fuzz) and passes installcheck-world, but a rebase is needed, because 3 lines of context aren't enough to get the doc changes in the right place in the aggregate function table. (I think generating the patch with 4 lines of context would be enough to keep that from being a recurring issue.) One thing that seems a bit funny is this message in the new multirange_agg_transfn: + if (!type_is_multirange(mltrngtypoid)) + ereport(ERROR, + (errcode(ERRCODE_DATATYPE_MISMATCH), + errmsg("range_agg must be called with a multirange"))); It's clearly copied from the corresponding test and message in range_agg_transfn. They both say "range_agg must be called ...", which makes perfect sense, as from the user's perspective both messages come from (different overloads of) a function named range_agg. Still, it could be odd to have (again from the user's perspective) a function named range_agg that sometimes says "range_agg must be called with a range" and other times says "range_agg must be called with a multirange". I'm not sure how to tweak the wording (of either message or both) to make that less weird, but there's probably a way. I kind of wonder whether either message is really reachable, at least through the aggregate machinery in the expected way. Won't that machinery ensure that it is calling the right transfn with the right type of argument? If that's the case, maybe the messages could only be seen by someone calling the transfn directly ... which also seems ruled out for these transfns because the state type is internal. Is this whole test more of the nature of an assertion? Regards, -Chap The new status of this patch is: Waiting on Author