Re: Combining Aggregates

On Mon, Mar 21, 2016 at 2:18 PM, David Rowley
<david(dot)rowley(at)2ndquadrant(dot)com> wrote:
> I've attached 2 of the patches which are affected by the changes.

I think the documentation for 0001 needs some work yet. The
additional paragraph that you've added...

(1) doesn't seem to appear at a very logical place in the
documentation - I think it should be much further down, as it's a
minor detail. Maybe document this the same way as the documentation
patch you just sent for the combine-function stuff does it; and

(2) isn't indented consistently with the surrounding paragraphs; and

(3) is missing a closing </para> tag

Also, I'd just cut this:

+ This is required due to
+ the process model being unable to pass references to <literal>INTERNAL
+ </literal> types between different <productname>PostgreSQL</productname>
+ processes.

Instead, I'd change the earlier sentence in the paragraph, which
currently reads:

+ These
+ functions are required in order to allow parallel aggregation for aggregates
+ with an <replaceable class="PARAMETER">stype</replaceable> of <literal>
+ INTERNAL</>.

I'd replace the period at end with a comma and add "since
<literal>INTERNAL</> values represent arbitrary in-memory data
structures which can't be passed between processes". I think that's a
bit smoother.

I'm going to read through the code again now.

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