On 2/26/22 17:13, Chapman Flack wrote:
> 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.)
Thank you for the review and the tip re 4 lines of context! Rebase attached.
> 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")));
I agree it would be more helpful to users to let them know we can take
either kind of argument. I changed the message to "range_agg must be
called with a range or multirange". How does that seem?
> 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?
I don't think they are reachable, so perhaps they are more like asserts.
Do you think I should change it? It seems like a worthwhile check in any
case.
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com
