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
