Re: EXPLAIN VERBOSE with parallel Aggregate

On 27 April 2016 at 15:12, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Apr 26, 2016 at 10:57 PM, David Rowley
> <david(dot)rowley(at)2ndquadrant(dot)com> wrote:
>> On 27 April 2016 at 14:30, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Tue, Apr 26, 2016 at 9:56 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>> On Tue, Apr 26, 2016 at 9:14 PM, David Rowley
>>>> <david(dot)rowley(at)2ndquadrant(dot)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.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services