On 17 March 2016 at 00:57, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Mar 16, 2016 at 6:49 AM, David Rowley
> <david.rowley@2ndquadrant.com> wrote:
>> On 16 March 2016 at 15:04, Robert Haas <robertmhaas@gmail.com> wrote:
>>> I don't think I'd be objecting if you made PartialAggref a real
>>> alternative to Aggref. But that's not what you've got here. A
>>> PartialAggref is just a wrapper around an underlying Aggref that
>>> changes the interpretation of it - and I think that's not a good idea.
>>> If you want to have Aggref and PartialAggref as truly parallel node
>>> types, that seems cool, and possibly better than what you've got here
>>> now. Alternatively, Aggref can do everything. But I don't think we
>>> should go with this wrapper concept.
>>
>> Ok, I've now gotten rid of the PartialAggref node, and I'm actually
>> quite happy with how it turned out. I made
>> search_indexed_tlist_for_partial_aggref() to follow-on the series of
>> other search_indexed_tlist_for_* functions and have made it behave the
>> same way, by returning the newly created Var instead of doing that in
>> fix_combine_agg_expr_mutator(), as the last version did.
>>
>> Thanks for the suggestion.
>>
>> New patch attached.
>
> Cool! Why not initialize aggpartialtype always?
Because the follow-on patch sets that to either the serialtype or the
aggtranstype, depending on if serialisation is required. Serialisation
is required for parallel aggregate, but if we're performing the
partial agg in the main process, then we'd not need to do that. This
could be solved by adding more fields to AggRef to cover the
aggserialtype and perhaps expanding aggpartial into an enum mode which
allows NORMAL, PARTIAL, PARTIAL_SERIALIZE, and have exprType() pay
attention to the mode and return 1 of the 3 possible types.
-- David Rowley http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
