Re: Parallel Index Scans

On Wed, Jan 18, 2017 at 8:03 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> 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?

Sure.

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

Hmm, tricky.  OK, I'll think about that some more.

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

It's pretty minor code duplication; I don't think it's an issue.

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

OK.

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