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.
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.)
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.
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.
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.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
