On Mon, Jan 16, 2017 at 7:11 AM, Amit Kapila <amit.kapil...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
