RE: Partial aggregates pushdown - Mailing list pgsql-hackers

Hi Mr.Pyhalov.

Thank you for comments.

> From: Alexander Pyhalov <a.pyhalov@postgrespro.ru>
> Sent: Tuesday, June 6, 2023 1:19 PM
> >> Have found one issue -
> >>
> >> src/backend/catalog/pg_aggregate.c
> >>
> >> 585                 if(strcmp(strVal(linitial(aggpartialfnName)),
> >> aggName) == 0){
> >> 586                         if(((aggTransType != INTERNALOID) &&
> >> (finalfn != InvalidOid))
> >> 587                                         || ((aggTransType ==
> >> INTERNALOID) && (finalfn != serialfn)))
> >> 588                                 elog(ERROR, "%s is not its own
> >> aggpartialfunc", aggName);
> >> 589                 } else {
> >>
> >> Here string comparison of aggName and aggpartialfnName looks very
> >> suspicios - it seems you should compare oids, not names (in this
> >> case, likely oids of transition function and partial aggregation function).
> >> The fact that aggregate name matches partial aggregation function
> >> name is not a enough to make any decisions.
> >
> > I see no problem with this string comparison. Here is the reason.
> > The intent of this code is, to determine whether the user specifies
> > the new aggregate function whose aggpartialfunc is itself.
> > For two aggregate functions,
> > If the argument list and function name match, then the two aggregate
> > functions must match.
> > By definition of aggpartialfunc,
> > every aggregate function and its aggpartialfn must have the same
> > argument list.
> > Thus, if aggpartialfnName and aggName are equal as strings, I think it
> > is correct to determine that the user is specifying the new aggregate
> > function whose aggpartialfunc is itself.
> >
> > However, since the document does not state these intentions I think
> > your suspicions are valid.
> > Therefore, I have added a specification to the document reflecting the
> > above intentions.
> >
> 
> Hi. Let me explain.
> 
> Look at this example, taken from test.
> 
> CREATE AGGREGATE udf_avg_p_int4(int4) (
>         sfunc = int4_avg_accum,
>         stype = _int8,
>         combinefunc = int4_avg_combine,
>         initcond = '{0,0}'
> );
> CREATE AGGREGATE udf_sum(int4) (
>         sfunc = int4_avg_accum,
>         stype = _int8,
>         finalfunc = int8_avg,
>         combinefunc = int4_avg_combine,
>         initcond = '{0,0}',
>         aggpartialfunc = udf_avg_p_int4
> );
> 
> Now, let's create another aggregate.
> 
> # create schema test ;
> create aggregate test.udf_avg_p_int4(int4) (
>         sfunc = int4_avg_accum,
>         stype = _int8,
>         finalfunc = int8_avg,
>         combinefunc = int4_avg_combine,
>         initcond = '{0,0}',
>         aggpartialfunc = udf_avg_p_int4
> );
> ERROR:  udf_avg_p_int4 is not its own aggpartialfunc
> 
> What's the difference between test.udf_avg_p_int4(int4) aggregate and
> udf_sum(int4)? They are essentially the same, but second one can't be
> defined.
> 
> Also note difference:
> 
> # CREATE AGGREGATE udf_sum(int4) (
>         sfunc = int4_avg_accum,
>         stype = _int8,
>         finalfunc = int8_avg,
>         combinefunc = pg_catalog.int4_avg_combine,
>         initcond = '{0,0}',
>         aggpartialfunc = udf_avg_p_int4
> );
> CREATE AGGREGATE
> 
> # CREATE AGGREGATE udf_sum(int4) (
>         sfunc = int4_avg_accum,
>         stype = _int8,
>         finalfunc = int8_avg,
>         combinefunc = pg_catalog.int4_avg_combine,
>         initcond = '{0,0}',
>         aggpartialfunc = public.udf_avg_p_int4 );
> ERROR:  aggpartialfnName is invalid
> 
> It seems that assumption about aggpartialfnName - that it's a non-qualified
> name is incorrect. And if we use qualified names, we can't compare just names,
> likely we should compare oids.

Thanks for the explanation.
I understand that the method of comparing two function name strings is incorrect.
Instead, I added the parameter isaggpartialfunc indicating whether the aggregate
function and its aggpartialfunc are the same or different.

Sincerely yours,
Yuuki Fujii

--
Yuuki Fujii
Information Technology R&D Center Mitsubishi Electric Corporation