Re: range_agg with multirange inputs

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