From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Anastasia Lubennikova <a(dot)lubennikova(at)postgrespro(dot)ru>, Anastasia Lubennikova <lubennikovaav(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Rahila Syed <rahilasyed(dot)90(at)gmail(dot)com> |
Subject: | Re: Parallel Index Scans |
Date: | 2017-02-15 12:11:18 |
Message-ID: | CAA4eK1+zo4O6ak5ew68SB5EZvZ1j2hxb4af3gaQ_h=qAg40vWg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Feb 15, 2017 at 2:04 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Feb 14, 2017 at 12:48 PM, Robert Haas <robertmhaas(at)gmail(dot)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.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachment | Content-Type | Size |
---|---|---|
parallel_index_scan_v10.patch | application/octet-stream | 26.8 KB |
guc_parallel_index_scan_v1.patch | application/octet-stream | 10.1 KB |
parallel_index_opt_exec_support_v10.patch | application/octet-stream | 37.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Dilip Kumar | 2017-02-15 12:17:58 | Re: Parallel bitmap heap scan |
Previous Message | Heikki Linnakangas | 2017-02-15 11:28:38 | Re: SCRAM authentication, take three |