Re: Parallel Aggregate

On 16 December 2015 at 18:11, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
wrote:

> On Tue, Dec 15, 2015 at 8:04 AM, Paul Ramsey <pramsey(at)cleverelephant(dot)ca>
> wrote:
> > But the run dies.
> >
> > NOTICE: SRID value -32897 converted to the officially unknown SRID
> value 0
> > ERROR: Unknown geometry type: 2139062143 - Invalid type
> >
> > From the message it looks like geometry gets corrupted at some point,
> > causing a read to fail on very screwed up metadata.
>
> Thanks for the test. There was some problem in advance_combination_function
> in handling pass by reference data. Here I attached updated patch with the
> fix.

Thanks for fixing this.

I've been playing around with this patch and looking at the code, and I
just have a few general questions that I'd like to ask to see if there's
any good answers for them yet.

One thing I noticed is that you're only enabling Parallel aggregation when
there's already a Gather node in the plan. Perhaps this is fine for a proof
of concept, but I'm wondering how we can move forward from this to
something that can be committed. As of how the patch is today, it means
that you don't get a parallel agg plan for;

Query 1: select count(*) from mybigtable;

but you do for;

Query 2: select count(*) from mybigtable where <some fairly selective
clause>;

since the <some fairly selective clause> allows parallel seq scan to win
over a seq scan as it does not have as much cost to moving tuples from the
worker process into the main process. If that Gather node is found the code
shuffles the plan around so that the partial agg node is below it and
sticks a Finalize Agg node below the Gather. I imagine you wrote this with
the intention of finding something better later, once we see that it all
can work, and cool, it seems to work!

I'm not quite sure what the general solution is to improve on this is this
yet, as it's not really that great if we can't get parallel aggregation on
Query 1, but we can on Query 2.

In master today we seem to aim to parallelise at the path level, which
seems fine if we only aim to have SeqScan as the only parallel enabled
node, but once we have several other nodes parallel enabled, we might, for
instance, want to start parallelising whole plan tree branches, if all
nodes in that branch happen to support parallelism.

I'm calling what we have today "keyhole parallelism", because we enable
parallelism while looking at a tiny part of the picture. I get the
impression that the bigger picture has been overlooked as perhaps it's more
complex and keyhole parallelism at least allows us to get something in
that's parallelised, but this patch indicates to me that we're already
hitting the limits of that, should we rethink? I'm concerned as I've come
to learn that changing is sort of thing after a release is much harder as
people start to find cases where performance regresses which makes it much
more difficult to improve things.

Also my apologies if I've missed some key conversation about how all of the
above is intended to work. Please feel free to kick me into line if that's
the case.

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