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>
 <621E912C.3020307@anastigmatix.net>
 <42454377-d914-0e22-37da-1b63ce3b9f9d@illuminatedcomputing.com>
From: Chapman Flack <chap@anastigmatix.net>
Message-ID: <622A7690.4020800@anastigmatix.net>
Date: Thu, 10 Mar 2022 17:07:12 -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: <42454377-d914-0e22-37da-1b63ce3b9f9d@illuminatedcomputing.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/622A7690.4020800%40anastigmatix.net>
Precedence: bulk

On 03/05/22 15:53, Paul Jungwirth wrote:
> 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!

When I apply this patch, I get a func.sgml with two entries for
range_intersect_agg(anymultirange).

> I like the elog solution. I've changed them in both places.

It looks like you've now got elog in three places: the "must be called
with a range or multirange" in multirange_agg_transfn and
multirange_intersect_agg_transfn, and the "called in non-aggregate
context" in multirange_agg_transfn.

I think that last is also ok, given that its state type is internal,
so it shouldn't be reachable in a user call.

In range_agg_transfn, you've changed the message in the "must be called
with a range or multirange"; that seems like another good candidate to
be an elog.

> 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.

I think your query finds aggregate declarations that share the same SQL
function declaration as their finalizer functions. That seems to be more
common.

The query I used looks for cases where different SQL-declared functions
appear as finalizers of aggregates, but the different SQL declared functions
share the same internal C implementation. That's the query where this seems
to be the unique result.

WITH
 finals(regp) AS (
  SELECT DISTINCT
    CAST(aggfinalfn AS regprocedure)
   FROM
    pg_aggregate
   WHERE
    aggfinalfn <> 0 -- InvalidOid
 )
SELECT
  prosrc, array_agg(regp)
 FROM
  pg_proc, finals
 WHERE
  oid = regp AND prolang = 12 -- INTERNALlanguageId
 GROUP BY
  prosrc
 HAVING
  count(*) > 1;

In other words, I think the interesting thing to say in the C comment
is not "shared by range_agg(anyrange) and range_agg(anymultirange)", but
"shared by range_agg_finalfn(internal,anyrange) and
multirange_agg_finalfn(internal,anymultirange)".

It seems a little extra surprising to have one C function declared in SQL
with two different names and parameter signatures. It ends up working
out because it relies on get_fn_expr_rettype, which can do its job for
either polymorphic type it might find in the parameter declaration.
But that's a bit subtle. :)

Regards,
-Chap