Re: Parallel Seq Scan

On Thu, Oct 1, 2015 at 7:52 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Wed, Sep 30, 2015 at 7:05 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Sep 24, 2015 at 2:31 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>> wrote:
>> > [ parallel_seqscan_partialseqscan_v18.patch ]
>>
>> I spent a bit of time reviewing the heapam.c changes in this patch
>> this evening, and I think that your attempt to add support for
>> synchronized scans has some problems.
>
> Thanks for the review and I agree with all the suggestions provided
> by you. Fixed all of them in attached patch
> (parallel_seqscan_heapscan_v1.patch).

Thanks.

Does heap_parallelscan_nextpage really need a pscan_finished output
parameter, or can it just return InvalidBlockNumber to indicate end of
scan? I think the latter can be done and would be cleaner.

There doesn't seem to be anything that ensures that everybody who's
scanning the relation agrees on whether we're doing a synchronized
scan. I think that heap_parallelscan_initialize() should taken an
additional Boolean argument, allow_sync. It should decide whether to
actually perform a syncscan using the logic from initscan(), and then
it should store a phs_syncscan flag into the ParallelHeapScanDesc.
heap_beginscan_internal should set rs_syncscan based on phs_syncscan,
regardless of anything else.

I think heap_parallel_rescan() is an unworkable API. When rescanning
a synchronized scan, the existing logic keeps the original start-block
so that the scan's results are reproducible, but no longer reports the
scan position since we're presumably out of step with other backends.
This isn't going to work at all with this API. I don't think you can
swap out the ParallelHeapScanDesc for another one once you've
installed it; the point of a rescan is that you are letting the
HeapScanDesc (or ParallelHeapScanDesc) hold onto some state from the
first time, and that doesn't work at all here. So, I think this
function should just go away, and callers should be able to just use
heap_rescan().

Now this presents a bit of a problem for PartialSeqScan, because, on a
rescan, nodeGather.c completely destroys the DSM and creates a new
one. I think we're going to need to change that. I think we can
adapt the parallel context machinery so that after
WaitForParallelWorkersToFinish(), you can again call
LaunchParallelWorkers(). (That might already work, but I wouldn't be
surprised if it doesn't quite work.) This would make rescans somewhat
more efficient because we wouldn't have to destroy and re-create the
DSM each time. It means that the DSM would have to stick around until
we're totally done with the query, rather than going away when
ExecGather() returns the last tuple, but that doesn't sound too bad.
We can still clean up the workers when we've returned all the tuples,
which I think is the most important thing.

This is obviously going to present some design complications for the
as-yet-uncommitted code to push down PARAM_EXEC parameters, because if
the new value takes more bytes to store than the old value, there
won't be room to update the existing DSM in place. There are several
possible solutions to that problem, but the one that appeals to me
most right now is just don't generate plans that would require that
feature. It doesn't seem particularly appealing to me to put a Gather
node on the inner side of a NestLoop -- at least not until we can
execute that without restarting workers, which we're certainly some
distance from today. And maybe not even then. For initPlans, the
existing code might be adequate, because I think we never re-evaluate
those. And for subPlans, we can potentially handle cases where each
worker can evaluate the subPlan separately below the Gather; we just
can't handle cases where the subPlan attaches above the Gather and is
used below it. Or, we can get around these limitations by redesigning
the PARAM_EXEC pushdown mechanism in some way. But even if we don't,
it's not crippling.

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