Subject: Re: range_agg with multirange inputs
To: Paul Jungwirth <pj@illuminatedcomputing.com>,
 pgsql-hackers@lists.postgresql.org
References: <007ef255-35ef-fd26-679c-f97e7a7f30c2@illuminatedcomputing.com>
 <007ef255-35ef-fd26-679c-f97e7a7f30c2@illuminatedcomputing.com>
 <164592443404.882.11296254043197567171.pgcf@coridan.postgresql.org>
 <6556b959-f839-aee2-4b4b-0b5107c0db11@illuminatedcomputing.com>
From: Chapman Flack <chap@anastigmatix.net>
Message-ID: <621E912C.3020307@anastigmatix.net>
Date: Tue, 1 Mar 2022 16:33:32 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.7.0
MIME-Version: 1.0
In-Reply-To: <6556b959-f839-aee2-4b4b-0b5107c0db11@illuminatedcomputing.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/621E912C.3020307%40anastigmatix.net>
Precedence: bulk

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