On 16 March 2016 at 13:42, Robert Haas <robertmh...@gmail.com> wrote: > On Tue, Mar 15, 2016 at 8:04 PM, David Rowley > <david.row...@2ndquadrant.com> wrote: >> On 16 March 2016 at 12:58, Robert Haas <robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
