Thread: BUG #3852: Could not create complex aggregate

The following bug has been logged online:

Bug reference:      3852
Logged by:          Sokolov Yura
Email address:      funny.falcon@gmail.com
PostgreSQL version: 8.3beta4
Operating system:   Linux Debian 4.0rel2
Description:        Could not create complex aggregate
Details:

Possibly it's not an error, then how can I make such aggregate?

create or replace function add_group(grp anyarray, ad anyelement, size int4)
returns anyarray
language plpgsql
as $$
begin
  if array_upper(grp, 1) < size then
    return grp||ad;
  end if;
  return grp;
end;
$$ immutable;

create aggregate build_group(anyelement, int4) (
  SFUNC= add_group,
  STYPE = anyarray
);

> ERROR:  argument declared "anyarray" is not an array but type anyarray
> SQL State:42804

"Sokolov Yura" <funny.falcon@gmail.com> writes:
> create or replace function add_group(grp anyarray, ad anyelement, size int4)
> returns anyarray
> language plpgsql
> ...

> create aggregate build_group(anyelement, int4) (
>   SFUNC= add_group,
>   STYPE = anyarray
> );

> ERROR:  argument declared "anyarray" is not an array but type anyarray

After chewing on this for awhile, it seems to me that pg_aggregate.c
is using enforce_generic_type_consistency() in a rather fundamentally
different way than it's being used anywhere else.  Everywhere else,
the "actual" argument types are expected to be real (not polymorphic)
types and enforce_generic_type_consistency() is expected to derive a
real result type.  But in pg_aggregate.c, the "actual" argument types
are the declared input and transition types of the aggregate function,
which could be polymorphic, and it is okay to hand back a polymorphic
result type if there's not enough information yet.

I think we could make enforce_generic_type_consistency() clearer by
adding an additional argument "bool allow_poly" which specifies
whether polymorphic "actual" argument and result types are allowed.
(Or maybe split it into two functions, although I think there'd be a lot
of code duplication.)  In this case it would allow ANYARRAY as the
"actual" argument type matching an ANYARRAY parameter, similarly for
ANYELEMENT, ANYENUM, etc, and it would hand back ANYARRAY or ANYELEMENT
if the result type couldn't be determined yet.  lookup_agg_function()
should always invoke enforce_generic_type_consistency(), with this
argument "true".

Although this problem really goes quite far back, I think it's probably
not interesting to back-patch further than 8.2, because AFAICS the
interesting cases involve aggregates with more than one argument.

Thoughts?
        regards, tom lane

Tom Lane wrote:
> "Sokolov Yura" <funny.falcon@gmail.com> writes:
>> create or replace function add_group(grp anyarray, ad anyelement, size int4)
>> returns anyarray
>> language plpgsql
>> ...
> 
>> create aggregate build_group(anyelement, int4) (
>>   SFUNC= add_group,
>>   STYPE = anyarray
>> );
> 
>> ERROR:  argument declared "anyarray" is not an array but type anyarray

> After chewing on this for awhile, it seems to me that pg_aggregate.c
> is using enforce_generic_type_consistency() in a rather fundamentally
> different way than it's being used anywhere else.
[snip]
> I think we could make enforce_generic_type_consistency() clearer by
> adding an additional argument "bool allow_poly" which specifies
> whether polymorphic "actual" argument and result types are allowed.
[snip]
> lookup_agg_function()
> should always invoke enforce_generic_type_consistency(), with this
> argument "true".

This sounds like a reasonable plan to me.

> Although this problem really goes quite far back, I think it's probably
> not interesting to back-patch further than 8.2, because AFAICS the
> interesting cases involve aggregates with more than one argument.

I agree, especially since this is the first time anyone has complained.

Did you want me to work on this? I could probably put some time into it 
this coming weekend.

Joe

Joe Conway <mail@joeconway.com> writes:
> Did you want me to work on this? I could probably put some time into it 
> this coming weekend.

I'll try to get to it before that --- if no serious bugs come up this
week, core is thinking of wrapping 8.3.0 at the end of the week, so
it'd be nice to have this dealt with sooner than that.
        regards, tom lane

Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
>   
>> Did you want me to work on this? I could probably put some time into it 
>> this coming weekend.
>>     
>
> I'll try to get to it before that --- if no serious bugs come up this
> week, core is thinking of wrapping 8.3.0 at the end of the week, so
> it'd be nice to have this dealt with sooner than that.
>
>             regards, tom lane
>
>   

CREATE AGGREGATE array_concat(anyarray) ( SFUNC=array_cat, STYPE=anyarray
);

CREATE AGGREGATE array_build(anyelement) ( SFUNC=array_append, STYPE=anyarray
);

Sorry for previous message having no comments.

Just remark:
These aggregates created successfuly both in 8.2 and 8.3beta4:

CREATE AGGREGATE array_concat(anyarray) (SFUNC=array_cat,STYPE=anyarray
);

CREATE AGGREGATE array_build(anyelement) (SFUNC=array_append,STYPE=anyarray
);

But aggregate from first letter does not:

create aggregate build_group(anyelement, int4) ( SFUNC= add_group, STYPE = anyarray
);


Excuse me for being noisy and bad English.

Joe Conway <mail@joeconway.com> writes:
> Tom Lane wrote:
>> I think we could make enforce_generic_type_consistency() clearer by
>> adding an additional argument "bool allow_poly" which specifies
>> whether polymorphic "actual" argument and result types are allowed.

> This sounds like a reasonable plan to me.

>> Although this problem really goes quite far back, I think it's probably
>> not interesting to back-patch further than 8.2, because AFAICS the
>> interesting cases involve aggregates with more than one argument.

> I agree, especially since this is the first time anyone has complained.

I've applied a patch along these lines, although I desisted from
back-patching it.  It seems a bit like a new feature, and also I'm not
100% sure we have all the bases covered even yet.
        regards, tom lane