Re: Parallel Seq Scan

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.

- In both heapgettup() and heapgettup_pagemode(), you call
ss_report_location() on discovering that we're trying to initialize
after the scan is already complete. This seems wrong. For the
reasons noted in the existing comments, it's good for the backend that
finishes the scan to report the starting position as the current
position hint, but you don't want every parallel backend to do it
turn. Unrelated, overlapping scans might be trying to continue
advancing the scan, and you don't want to drag the position hint
backward for no reason.

- heap_parallelscan_initialize_startblock() calls ss_get_location()
while holding a spinlock. This is clearly no good, because spinlocks
can only be held while executing straight-line code that does not
itself acquire locks - and ss_get_location() takes an *LWLock*. Among
other problems, an error anywhere inside ss_get_location() would leave
behind a stuck spinlock.

- There's no point that I can see in initializing rs_startblock at all
when a ParallelHeapScanDesc is in use. The ParallelHeapScanDesc, not
rs_startblock, is going to tell you what page to scan next.

I think heap_parallelscan_initialize_startblock() should basically do
this, in the synchronized scan case:

SpinLockAcquire(&parallel_scan->phs_mutex);
page = parallel_scan->phs_startblock;
SpinLockRelease(&parallel_scan->phs_mutex);
if (page != InvalidBlockNumber)
return; /* some other process already did this */
page = ss_get_location(scan->rs_rd, scan->rs_nblocks);
SpinLockAcquire(&parallel_scan->phs_mutex);
/* even though we checked before, someone might have beaten us here */
if (parallel_scan->phs_startblock == InvalidBlockNumber)
{
parallel_scan->phs_startblock = page;
parallel_scan->phs_cblock = page;
}
SpinLockRelease(&parallel_scan->phs_mutex);

- heap_parallelscan_nextpage() seems to have gotten unnecessarily
complicated. I particularly dislike the way you increment phs_cblock
and then sometimes try to back it out later. Let's decide that
phs_cblock == InvalidBlockNumber means that the scan is finished,
while phs_cblock == phs_startblock means that we're just starting. We
then don't need phs_firstpass at all, and can write:

SpinLockAcquire(&parallel_scan->phs_mutex);
page = parallel_scan->phs_cblock;
if (page != InvalidBlockNumber)
{
parallel_scan->phs_cblock++;
if (parallel_scan->phs_cblock >= scan->rs_nblocks)
parallel_scan->phs_cblock = 0;
if (parallel_scan->phs_cblock == parallel_scan->phs_startblock)
{
parallel_scan->phs_cblock = InvalidBlockNumber;
report_scan_done = true;
}
}
SpinLockRelease(&parallel_scan->phs_mutex);

At this point, if page contains InvalidBlockNumber, then the scan is
done, and if it contains anything else, that's the next page that the
current process should scan. If report_scan_done is true, we are the
first to observe that the scan is done and should call
ss_report_location().

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