On Tue, Mar 15, 2016 at 8:55 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
> On 16 March 2016 at 13:42, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Tue, Mar 15, 2016 at 8:04 PM, David Rowley
>> <david.rowley@2ndquadrant.com> wrote:
>>> On 16 March 2016 at 12:58, Robert Haas <robertmhaas@gmail.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