On Tue, Apr 26, 2016 at 11:38 PM, David Rowley <david.row...@2ndquadrant.com> wrote: > On 27 April 2016 at 15:12, Robert Haas <robertmh...@gmail.com> wrote: >> On Tue, Apr 26, 2016 at 10:57 PM, David Rowley >> <david.row...@2ndquadrant.com> wrote: >>> On 27 April 2016 at 14:30, Robert Haas <robertmh...@gmail.com> wrote: >>>> On Tue, Apr 26, 2016 at 9:56 PM, Robert Haas <robertmh...@gmail.com> wrote: >>>>> On Tue, Apr 26, 2016 at 9:14 PM, David Rowley >>>>> <david.row...@2ndquadrant.com> wrote: >>>>>> I'd also have expected the output of both partial nodes to be the >>>>>> same, i.e. both prefixed with PARTIAL. Is it intended that they don't? >>>>>> or have I made some other mistake? >>>>> >>>>> No, that's a defect in the patch. I didn't consider that we need to >>>>> support nodes with finalizeAggs = false and combineStates = true, >>>>> which is why that ERROR was there. Working on a fix now. >>>> >>>> I think this version should work, provided you use >>>> partial_grouping_target where needed. >>> >>> +static void get_special_variable(Node *node, deparse_context *context, >>> + void *private); >>> >>> "private" is reserved in C++? I understood we want our C code to >>> compile as C++ too, right? or did I get my wires crossed somewhere? >> >> I can call it something other than "private", if you have a >> suggestion; normally I would have used "context", but that's already >> taken in this case. private_context would work, I guess. > > It's fine. After Andres' email I looked and saw many other usages of > C++ keywords in our C code. Perhaps it would be a good idea to name it > something else we wanted to work towards it, but it sounds like it's > not, so probably keep what you've got. > > The patch looks good. The only other thing I thought about was perhaps > it would be a good idea to re-add the sanity checks in execQual.c. > Patch for that is attached. > > I removed the aggoutputtype check, as I only bothered adding that in > the first place because I lost the aggpartial field in some previous > revision of the parallel aggregate developments. I'd say the > aggpartial check makes it surplus to requirements.
OK, committed. -- 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
