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