Re: Partial aggregates pushdown - Mailing list pgsql-hackers

On Wed, Aug 21, 2024 at 04:59:12PM +0200, Tomas Vondra wrote:
> On 8/20/24 20:41, Bruce Momjian wrote:
> >     SELECT (oid, relname) FROM pg_class LIMIT 1;
> >              row
> >     ---------------------
> >      (2619,pg_statistic)
> > 
> >     SELECT pg_typeof((oid, relname)) FROM pg_class LIMIT 1;
> >      pg_typeof
> >     -----------
> >      record
> > 
> >     SELECT pg_typeof(oid) FROM pg_class LIMIT 1;
> >      pg_typeof
> >     -----------
> >      oid
> >     
> >     SELECT pg_typeof(relname) FROM pg_class LIMIT 1;
> >      pg_typeof
> >     -----------
> >      name
> > 
> 
> How would this help with the AVG(bigint) case? We don't have int128 as
> SQL type, so what would be part of the record? Also, which part of the
> code would produce the record? If the internal state is "internal", that
> would probably need to be something aggregate specific, and that's kinda
> what this patch series is adding, no?
> 
> Or am I missing some cases where the record would make it work?

Right now, my goal in this thread is to try to concretely explain what
is being proposed.  Therefore, I think I need to go back and make four
categories instead of two:

1.  cases like MAX(int), where we return only one value, and the FDW
return value is an existing data type, e.g., int

2.  cases like AVG(int) where we return multiple FDW values of the same
type and can use an array, e.g.,  bigint array

3.  cases like AVG(bigint) where we return multiple FDW values of the
same type (or can), but the one of the FDW return values is not an
existing data type, e.g.  int128

4.  cases like AVG(interval) where we return multiple FDW values of
different types, e.g. interval and an integral count

For #1, all MAX cases have aggregate input parameters the same as the
FDW return types (aggtranstype):

    SELECT proargtypes[0]::regtype, aggtranstype::regtype
    FROM pg_aggregate a JOIN pg_proc p ON a.aggfnoid = p.oid
    WHERE proname = 'max' AND proargtypes[0] != aggtranstype;

     proargtypes | aggtranstype
    -------------+--------------

For #2-4, we have for AVG:

    SELECT proargtypes[0]::regtype, aggtranstype::regtype
    FROM pg_aggregate a JOIN pg_proc p ON a.aggfnoid = p.oid
    WHERE proname = 'avg';
    
       proargtypes    |    aggtranstype
    ------------------+--------------------
3->     bigint           | internal
2->     integer          | bigint[]
2->     smallint         | bigint[]
3->     numeric          | internal
2->     real             | double precision[]
2->     double precision | double precision[]
4->     interval         | internal

You can see which AVG items fall into which categories.  It seems we
have #1 and #2 handled cleanly in the patch.

My question is related to #3 and #4.  For #3, if we are going to be
building infrastructure to handle passing int128 for AVG, wouldn't it be
wiser to create an int128 type and an int128 array type, and then use
method #2 to handle those, rather than creating custom code just to
read/write int128 values for FDWs aggregate passing alone.

For #4, can we use or improve the RECORD data type to handle #4 --- that
seems preferable to creating custom FDWs aggregate passing code.

I know the open question was whether we should create custom FDWs
aggregate passing functions or custom data types for FDWs aggregate
passing, but I am asking if we can improve existing facilities, like
int128 or record passing, to reduce the need for some of these.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.