From: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Parallelized polymorphic aggs, and aggtype vs aggoutputtype |
Date: | 2016-06-20 08:10:53 |
Message-ID: | CAKJS1f8SQvemtvWOPjC9ync49YZEqnxPjotRuv3aLeb3O_KH9A@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 18 June 2016 at 09:29, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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
Attachment | Content-Type | Size |
---|---|---|
aggref_comments.patch | application/octet-stream | 1.5 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tatsuro Yamada | 2016-06-20 08:31:31 | Re: ERROR: ORDER/GROUP BY expression not found in targetlist |
Previous Message | Etsuro Fujita | 2016-06-20 08:10:44 | Push down join with PHVs (WAS: Re: [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116) |