On Tue, Jan 17, 2017 at 11:27 PM, Robert Haas <robertmh...@gmail.com> wrote: > 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. >
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
parallel_index_scan_v5.patch
Description: Binary data
parallel_index_opt_exec_support_v5.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers