Re: EXPLAIN VERBOSE with parallel Aggregate

On Tue, Apr 26, 2016 at 11:38 PM, David Rowley
<david(dot)rowley(at)2ndquadrant(dot)com> wrote:
> 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.

OK, committed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company