Re: Parallel Seq Scan

On Thu, Oct 1, 2015 at 7:41 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>
> 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.
>

I think we can do that way as well, however we have to keep the check
of page == InvalidBlockNumber at all the callers to indicate finish
of scan which made me write the function as it exists in patch. However,
I don't mind changing it the way you have suggested if you feel that 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 is implicitly ensured. We perform sync scan's based
on GUC synchronize_seqscans and few other conditions in initscan
which I think will ensure that all workers will perform sync scans
on relation. Do you see any problem with exiting logic which would
break the guarantee of sync scans on a relation among parallel
workers?

> 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 this will ensure that any future change in this area won't break the
guarantee for sync scans for parallel workers, is that the reason you
prefer to implement this function in the way suggested by you?

> 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,

It seems from the code comments in initscan, the reason for keeping
previous startblock is to allow rewinding the cursor which doesn't hold for
parallel scan. We might or might not want to support such cases with
parallel query, but even if we want to there is a way we can do with
current logic (as mentioned in one of my replies below).

> but no longer reports the
> scan position since we're presumably out of step with other backends.

Is it true for all form of rescans or are you talking about some
case (like SampleScan) in particular? As per my reading of code
(and verified by debugging that code path), it doesn't seem to be true
for rescan in case of seqscan.

> 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;
>

Sure, but if we really need some such parameters like startblock position,
then we can preserve those in ScanDesc.

>
> 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.
>

PARAM_EXEC parameters will be used to support initPlan in parallel query (as
it is done in the initial patch), so it seems to me this is the main
downside of
this approach. I think rather than trying to come up with various possible
solutions for this problem, lets prohibit sync scans with parallel query if
you
see some problem with the suggestions made by me above. Do you see any
main use case getting hit due to non support of sync scans with
parallel seq. scan?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com