Re: Parallel Index Scans

On Tue, Jan 17, 2017 at 11:27 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Jan 16, 2017 at 7:11 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> Fixed.
>
> Thanks for the update. Some more comments:
>
> It shouldn't be necessary for MultiExecBitmapIndexScan to modify the
> IndexScanDesc. That seems really broken. If a parallel scan isn't
> supported here (and I'm sure that's the case right now) then no such
> IndexScanDesc should be getting created.
>

Fixed.

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

Changed as per discussion.

> Why do we need separate AM support for index_parallelrescan()? Why
> can't index_rescan() cover that case? If the AM-specific method is
> getting the IndexScanDesc, it can see for itself whether it is a
> parallel scan.
>

Left as it is based on yesterdays discussion.

> I'd rename PS_State to BTPS_State, to match the other renamings.
>
> If we're going to update all of the AMs to set the new support
> functions to NULL, we should also update contrib/bloom.
>
> index_parallelscan_estimate has multiple lines that go over 80
> characters for no really good reason. Separate the initialization of
> index_scan from the variable declaration. Do the same for
> amindex_size. Also, you don't really need to encase the end of the
> function in an "else" block when the "if" block is guaranteed to
> returrn.
>
> Several function header comments still use the style where the first
> word of the description is "It". Say "this function" or similar the
> first time, instead of "it". Then when you say "it" later, it's clear
> that it refers back to where you said "this function".
>
> index_parallelscan_initialize also has a line more than 80 characters
> that looks easy to fix by splitting the declaration from the
> initialization.
>

Fixed all the above.

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

Changed as per yesterday's discussion.

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