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