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.
Should I update the patch to use the method you describe?
-- David Rowley http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services