Re: Cannot create an aggregate function with variadic parameters and enabled for parallel execution

Alexey Bashtanov <bashtanov@imap.cc> writes:
> An easier way to reproduce the problem:
> create aggregate weird_concat (foo variadic "any") (
>      sfunc = concat_ws,
>      stype = text,
>      combinefunc = textcat
> );

Thanks for the self-contained test case.  I concur this is broken.

The question we need to consider before fixing this, I think, is whether
we need to do anything immediately about the situation for polymorphic
aggregates.  If we just push your fix as-is, we'll be creating a
backwards compatibility constraint, so we had better get it right.

I believe that in fact it's okay, or at least we can define it as okay.
There are two components of the problem: can the support functions be
declared legally, and do they need additional type info at runtime
to do their work?

For the combine function, it seems fine to just say that the function
signature is alway "combine(stype, stype) returns stype" even in
polymorphic cases.  "combine(anyarray, anyarray) returns anyarray"
is a perfectly legal function declaration, so there's no problem at
that end.  At runtime, the combine function could inspect its argument
to find out what the array element type is.  In cases that are like
"combine(internal, internal) returns internal", the function declaration
is still acceptable.  The representation of the internal-type state
value would need to contain enough information for the combine function
to do its work without additional type lookups --- but that would likely
need to be true anyway, so it does not seem like a big penalty.

For the serial/deserial functions, the function signatures are
prespecified and don't depend on the aggregate argument types, so
there's no declaration issue.  Again, the state value would need
to be sufficiently self-contained for the functions to not need
additional info to figure out how to serialize it --- but again,
I do not see much problem there.

So I think we're okay with solving this as per your patch, though
it needs some more comments IMO.  Will push.

BTW, I noticed while looking at this that this error message in
nodeAgg.c seems to be exactly backwards:

        /*
         * Ensure that a combine function to combine INTERNAL states is not
         * strict. This should have been checked during CREATE AGGREGATE, but
         * the strict property could have been changed since then.
         */
        if (pertrans->transfn.fn_strict && aggtranstype == INTERNALOID)
            ereport(ERROR,
                    (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
                     errmsg("combine function for aggregate %u must be declared as STRICT",
                            aggref->aggfnoid)));
    }

Should be "must *not* be declared STRICT", no?  I wonder also why this
message isn't worded exactly like the corresponding one in pg_aggregate.c,
which says

                     errmsg("combine function with transition type %s must not be declared STRICT",
                            format_type_be(aggTransType))));

It doesn't really seem worth making translators expend effort on a variant
wording that has clearly never once been seen by a user (or they'd have
reported this as a bug).

            regards, tom lane