On 15/05/18 17:52, Tom Lane wrote:
>
> 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.
Sounds reasonable, aggregate arguments type info is not necessary for
combine/de-/serialize declarations, and for execution it could be
passed in the state variable.
> So I think we're okay with solving this as per your patch, though
> it needs some more comments IMO. Will push.
Do you mean it needs to be discussed more, or it needs more
inline code comments?
>
> 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?
Agree, this must have been a typo.
