On Wed, Feb 15, 2017 at 2:04 AM, Robert Haas <robertmh...@gmail.com> wrote: > On Tue, Feb 14, 2017 at 12:48 PM, Robert Haas <robertmh...@gmail.com> wrote: >> That sounds way better. > > Here's an updated patch. Please review my changes, which include: > > * Various comment updates.
1. + * BTPARALLEL_IDLE indicates that no backend is currently advancing the scan + * to a new page; some process can start doing that. + * + * BTPARALLEL_DONE implies that the scan is complete (including error exit). /implies/indicates, to be consistent with other explanations. 2. + * of the scan (depending on thes can direction). An invalid block number /thes can/the scan I have modified the patch to include above two changes. 3. + else if (pageStatus == BTPARALLEL_DONE) + { + /* + * We're done with this set of scankeys, but have not yet advanced + * to the next set. + */ + status = false; + } Here second part of the comment (but have not yet advanced ..) seems to be slightly misleading because this state has nothing to do with the advancement of scan keys. I have not changed this because I am not sure what you have in mind. > * _bt_parallel_seize now unconditionally sets *pageno to P_NONE at the > beginning, instead of doing it conditionally at the end. This seems > cleaner to me. > * I removed various BTScanPosInvalidate calls from _bt_first in places > where they followed calls to _bt_parallel_done, because I can't see > how the scan position could be valid at that point; note that > _bt_first asserts that it is invalid on entry. > * I added a _bt_parallel_done() call to _bt_first where it apparently > returned without releasing the scan; search for SK_ROW_MEMBER. Maybe > there's something I'm missing that makes this unnecessary, but if so > there should probably be a comment here. > * I wasn't happy with the strange calling convention where > _bt_readnextpage usually gets a valid block number but not for > non-parallel backward scans. I had a stab at fixing that so that the > block number is always valid, but I'm not entirely sure I've got the > logic right. Please see what you think. > Looks good to me. > * I repositioned the function prototypes you added to nbtree.h to > separate the public and non-public interfaces. > I have verified all your changes and they look good to me. > I can't easily test this because your second patch doesn't apply, I have tried and it works for me on latest code except for one test output file which could have been excluded. I wonder whether you are first applying the GUC related patch [1] before applying the optimizer support related patch. In anycase, to avoid confusion I am attaching all the three patches with this e-mail. > so > I'd appreciate it if you could have a stab at checking whether I've > broken anything in this revision. Also, it would be good if you could > rebase the second patch. > I have rebased the optimizer/executor support related patch. [1] - https://www.postgresql.org/message-id/CAA4eK1%2BTnM4pXQbvn7OXqam%2Bk_HZqb0ROZUMxOiL6DWJYCyYow%40mail.gmail.com -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
parallel_index_scan_v10.patch
Description: Binary data
guc_parallel_index_scan_v1.patch
Description: Binary data
parallel_index_opt_exec_support_v10.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