On 02/28/22 23:31, Paul Jungwirth wrote:
> On 2/26/22 17:13, Chapman Flack wrote:
>> (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.
I think the 4 lines should suffice, but it looks like this patch was
generated from a rebase of the old one (with three lines) that ended up
putting the new 'range_agg' entry ahead of 'max' in func.sgml, which
position is now baked into the 4 lines of context. :)
So I think it needs a bit of manual attention to get the additions back
in the right places, and then a 4-context-lines patch generated from that.
> I changed the message to "range_agg must be called
> with a range or multirange". How does that seem?
That works for me.
>> 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.
I would not change them to actual Assert, which would blow up the whole
process on failure. If it's a genuine "not expected to happen" case,
maybe changing it to elog (or ereport with errmsg_internal) would save
a little workload for translators. But as you were copying an existing
ereport with a translatable message, there's also an argument for sticking
to that style, and maybe mentioning the question to an eventual committer
who might have a stronger opinion.
I did a small double-take seeing the C range_agg_finalfn being shared
by the SQL range_agg_finalfn and multirange_agg_finalfn. I infer that
the reason it works is get_fn_expr_rettype works equally well with
either parameter type.
Do you think it would be worth adding a comment at the C function
explaining that? In a quick query I just did, I found no other aggregate
final functions sharing a C function that way, so this could be the first.
Regards,
-Chap
