On Tue, Mar 15, 2016 at 9:23 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
>>> 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.
>
> Not strongly. It means that Aggref will need another field to store
> the transtype and/or the serialtype (for the follow-on patches in
> Combining Aggregates thread)
> The only other issue which I don't think I've addressed yet is target
> list width estimates. Probably that can just pay attention to the
> aggpartial flag too, and if I fix that then likely the Aggref needs to
> carry around a aggtransspace field too, which we really only need to
> populate when the Aggref is in partial mode... it would be wasteful to
> bother looking that up from the catalogs if we're never going to use
> the Aggref in partial mode, and it seems weird to leave it as zero and
> only populate it when we need it. So... if I put on my object oriented
> hat and think about this.... My brain says that Aggref should inherit
> from PartialAggref.... and that's what I have now... or at least some
> C variant. So I really do think it's cleaner and easier to understand
> by keeping the ParialAggref.
>
> I see on looking at exprType() that we do have other node types which
> conditionally return a different type, but seeing that does not fill
> me with joy.
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.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company