Re: Parallel Index Scans

On Tue, Jan 17, 2017 at 11:27 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Jan 16, 2017 at 7:11 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
>
> WAIT_EVENT_PARALLEL_INDEX_SCAN is in fact btree-specific.  There's no
> guarantee that any other AMs the implement parallel index scans will
> use that wait event, and they might have others instead.  I would make
> it a lot more specific, like WAIT_EVENT_BTREE_PAGENUMBER.  (Waiting
> for the page number needed to continue a parallel btree scan to become
> available.)
>

WAIT_EVENT_BTREE_PAGENUMBER - NUMBER sounds slightly inconvenient. How
about just WAIT_EVENT_BTREE_PAGE?  We can keep the description as
suggested by you?

> Why do we need separate AM support for index_parallelrescan()?  Why
> can't index_rescan() cover that case?

The reason is that sometime index_rescan is called when we have to
just update runtime scankeys in index and we don't want to reset
parallel scan for that.  Refer ExecReScanIndexScan() changes in patch
parallel_index_opt_exec_support_v4.  Rescan is called from below place
during index scan.

ExecIndexScan(IndexScanState *node)
{
/*
* If we have runtime keys and they've not already been set up, do it now.
*/
if (node->iss_NumRuntimeKeys != 0 && !node->iss_RuntimeKeysReady)
ExecReScan((PlanState *) node);

>  If the AM-specific method is
> getting the IndexScanDesc, it can see for itself whether it is a
> parallel scan.
>

I think if we want to do that way then we need to pass some additional
information related to runtime scan keys in index_rescan method and
then probably to amspecific rescan method. That sounds scary.


>
> I think it's a bad idea to add a ParallelIndexScanDesc argument to
> index_beginscan().  That function is used in lots of places, and
> somebody might think that they are allowed to actually pass a non-NULL
> value there, which they aren't: they must go through
> index_beginscan_parallel.  I think that the additional argument should
> only be added to index_beginscan_internal, and
> index_beginscan_parallel should remain unchanged.
>

If we go that way then we need to set few parameters like heapRelation
and xs_snapshot in index_beginscan_parallel as we are doing in
index_beginscan. Does going that way sound better to you?

>  Either that, or get
> rid of index_beginscan_parallel altogether and have everyone use
> index_beginscan directly, and put the snapshot-restore logic in that
> function.
>

I think there is value in retaining index_beginscan_parallel as that
is parallel to heap_beginscan_parallel.


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