Re: Partial aggregates pushdown

Fujii.Yuki@df.MitsubishiElectric.co.jp писал 2023-06-06 06:08:
> Hi Mr.Pyhalov.
> 
> Thank you for your always thoughtful review.
> 
>> From: Alexander Pyhalov <a.pyhalov@postgrespro.ru>
>> Sent: Monday, June 5, 2023 6:00 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.

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional