Re: Parallelized polymorphic aggs, and aggtype vs aggoutputtype

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

In response to

Browse pgsql-hackers by date

  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)