On 3/1/22 13:33, Chapman Flack wrote:
> 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. :)
You're right, my last rebase messed up the docs. Here it is fixed. Sorry
about that!
> 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.
I like the elog solution. I've changed them in both places.
> 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.
I see 13 other shared finalfns (using select
array_agg(aggfnoid::regproc) as procs, array_agg(aggtransfn) as
transfns, aggfinalfn from pg_aggregate where aggfinalfn is distinct from
0 group by aggfinalfn having count(*) > 1;) but a comment can't hurt! Added.
Thanks,
--
Paul ~{:-)
pj@illuminatedcomputing.com
