On 18 June 2016 at 09:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So at this point my proposal is:
>
> 1. Add an OID-list field to Aggref holding the data types of the
> user-supplied arguments. This can be filled in parse analysis since it
> won't change thereafter. Replace calls to get_aggregate_argtypes() with
> use of the OID-list field, or maybe keep the function but have it look
> at the OID list not the physical args list.
>
> 2. Add an OID field to Aggref holding the resolved (non polymorphic)
> transition data type's OID. For the moment we could just set this
> during parse analysis, since we do not support changing the transtype
> of an existing aggregate. If we ever decide we want to allow that,
> the lookup could be postponed into the planner. Replace calls to
> resolve_aggregate_transtype with use of this field.
> (resolve_aggregate_transtype() wouldn't disappear, but it would be
> invoked only once during parse analysis, not multiple times per query.)
>
> #2 isn't necessary to fix the bug, but as long as we are doing #1
> we might as well do #2 too, to buy back some of the planner overhead
> added by parallel query.
>
> Barring objections I'll make this happen by tomorrow.
Thanks for committing this change.
>From reading over the commit I see you've left;
+ * XXX need more documentation about partial aggregation here
Would the attached cover off what you had imagined might go here?
> I still don't like anything about the way that the matching logic in
> fix_combine_agg_expr works, but at the moment I can't point to any
> observable bug from that, so I'll not try to change it before beta2.
Right, I see more work is needed in that area as there's now an
out-of-date comment at the top of
search_indexed_tlist_for_partial_aggref. The comment claims we only
ignore aggoutputtype, but you added aggtranstype and aggargtypes to
that list.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
