From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> |
Cc: | Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>, Paul Ramsey <pramsey(at)cleverelephant(dot)ca>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Parallel Aggregate |
Date: | 2016-03-16 01:08:29 |
Message-ID: | CA+TgmoajntJp6o3o3HDqETb=UbGjHBj1hPM_AhqKgBLdu_nDyg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Mar 15, 2016 at 8:55 PM, David Rowley
<david(dot)rowley(at)2ndquadrant(dot)com> wrote:
> On 16 March 2016 at 13:42, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Tue, Mar 15, 2016 at 8:04 PM, David Rowley
>> <david(dot)rowley(at)2ndquadrant(dot)com> wrote:
>>> On 16 March 2016 at 12:58, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>> ...and why would one be true and the other false?
>>>
>>> One would be the combine aggregate (having aggpartial = false), and
>>> the one in the subnode would be the partial aggregate (having
>>> aggpartial = true)
>>> Notice in create_grouping_paths() I build a partial aggregate version
>>> of the PathTarget named partial_group_target, this one goes into the
>>> partial agg node, and Gather node. In this case the aggpartial will be
>>> set differently for the Aggrefs in each of the two PathTargets, so it
>>> would not be possible in setrefs.c to find the correct target list
>>> entry in the subnode by using equal(). It'll just end up triggering
>>> the elog(ERROR, "Aggref not found in subplan target list"); error.
>>
>> OK, I get it now. I still don't like it very much. There's no
>> ironclad requirement that we use equal() here as opposed to some
>> bespoke comparison function with the exact semantics we need, and ISTM
>> that getting rid of PartialAggref would shrink this patch down quite a
>> bit.
>
> Well that might work. I'd not thought of doing it that way. The only
> issue that I can foresee with that is that when new fields are added
> to Aggref in the future, we might miss updating that custom comparison
> function to include them.
That's true, but it doesn't seem like that big a deal. A code comment
in the Aggref definition seems like sufficient insurance against such
a mistake.
> Should I update the patch to use the method you describe?
Well, my feeling is that is going to make this a lot smaller and
simpler, so I think so. But if you disagree strongly, let's discuss
further.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From | Date | Subject | |
---|---|---|---|
Next Message | Thomas Munro | 2016-03-16 01:13:48 | Re: Support for N synchronous standby servers - take 2 |
Previous Message | Amit Langote | 2016-03-16 00:56:31 | Re: [PROPOSAL] VACUUM Progress Checker. |