Re: Partial aggregates pushdown

Fujii(dot)Yuki(at)df(dot)MitsubishiElectric(dot)co(dot)jp писал 2023-06-06 06:08:
> Hi Mr.Pyhalov.
>
> Thank you for your always thoughtful review.
>
>> From: Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)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