On 27 March 2018 at 12:49, Tom Lane <t...@sss.pgh.pa.us> wrote: > I wrote: >> The main thing that remains undone is to get some test coverage --- >> AFAICS, none of these new functions get exercised in the standard >> regression tests. > > Oh, I thought of another thing that would need to get done, if we decide > to commit this. array_agg_serialize/deserialize only work if the array > element type has send/receive functions. The planner's logic to decide > whether partial aggregation is possible doesn't look any deeper than > whether the aggregate has serialize/deserialize, but I think we have to > teach it that if it's these particular serialize/deserialize functions, > it needs to check the array element type. Otherwise we'll get runtime > failures if partial aggregation is attempted on array_agg(). > > This would not matter for any core datatypes, since they all have > send/receive functions, but I imagine it's still pretty common for > extension types not to. > > For my money it'd be sufficient to hard-code the test like > > if ((aggserialfn == F_ARRAY_AGG_SERIALIZE || > aggdeserialfn == F_ARRAY_AGG_DESERIALIZE) && > !array_element_type_has_send_and_recv(exprType(aggregate input)))
I'd failed to consider this. Would it not be cleaner to just reject trans types without a send/receive function? Or do you think that would exclude other valid cases? -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
