Parallel Index-only scan

Please find the attached file for the latest version of patch.

parallel_index_only_v2.patch
<http://postgresql.nabble.com/file/n5936010/parallel_index_only_v2.patch&gt;

--
View this message in context: http://postgresql.nabble.com/Parallel-Index-only-scan-tp5934352p5936010.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Dec 23, 2016 at 12:04 AM, rafia.sabih
<rafia.sabih@enterprisedb.com> wrote:

Please find the attached file for the latest version of patch.

parallel_index_only_v2.patch
<http://postgresql.nabble.com/file/n5936010/parallel_index_only_v2.patch&gt;

We want to have the patch actually attached, not just stored on nabble.

--
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

Robert Haas wrote:

On Fri, Dec 23, 2016 at 12:04 AM, rafia.sabih
<rafia.sabih@enterprisedb.com> wrote:

Please find the attached file for the latest version of patch.

parallel_index_only_v2.patch
<http://postgresql.nabble.com/file/n5936010/parallel_index_only_v2.patch&gt;

We want to have the patch actually attached, not just stored on nabble.

I think the problem is that Nabble removes the attachment before
resending the email to the list. The OP should be posting directly
instead of via Nabble.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Dec 23, 2016 at 12:04 AM, rafia.sabih
<rafia.sabih@enterprisedb.com> wrote:

Please find the attached file for the latest version of patch.
parallel_index_only_v2.patch
<http://postgresql.nabble.com/file/n5936010/parallel_index_only_v2.patch&gt;

We want to have the patch actually attached, not just stored on nabble.

Or in words of one syllable: please do not use nabble to post to the
community mailing lists. Their habit of stripping attachments is not
the only evil thing about them (although it's the only one I remember
details of ATM).

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Dec 23, 2016 at 3:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Or in words of one syllable: please do not use nabble to post to the
community mailing lists.

Many of those words have two syllables, and one has four.

Anyhow, I think three castigating emails from committers in a span of
62 minutes is probably enough for the OP to get the point, unless
someone else REALLY feels the need to pile on.

--
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

Extremely sorry for the inconvenience caused, please find the attached file
for the latest version of the patch.

On Sat, Dec 24, 2016 at 1:41 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Dec 23, 2016 at 3:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Or in words of one syllable: please do not use nabble to post to the
community mailing lists.

Many of those words have two syllables, and one has four.

Anyhow, I think three castigating emails from committers in a span of
62 minutes is probably enough for the OP to get the point, unless
someone else REALLY feels the need to pile on.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/

Rebased patch of parallel-index only scan based on the latest version of
parallel index scan [1]/messages/by-id/CAA4eK1LiNi7_Z1%25 2BPCV4y06o_v%3DZdZ1UThE%2BW9JhthX4B8uifnA%40mail.gmail.com is attached.

[1]: /messages/by-id/CAA4eK1LiNi7_Z1%25 2BPCV4y06o_v%3DZdZ1UThE%2BW9JhthX4B8uifnA%40mail.gmail.com
2BPCV4y06o_v%3DZdZ1UThE%2BW9JhthX4B8uifnA%40mail.gmail.com

On Sat, Dec 24, 2016 at 7:55 PM, Rafia Sabih <rafia.sabih@enterprisedb.com>
wrote:

Extremely sorry for the inconvenience caused, please find the attached
file for the latest version of the patch.

On Sat, Dec 24, 2016 at 1:41 AM, Robert Haas <robertmhaas@gmail.com>
wrote:

On Fri, Dec 23, 2016 at 3:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Or in words of one syllable: please do not use nabble to post to the
community mailing lists.

Many of those words have two syllables, and one has four.

Anyhow, I think three castigating emails from committers in a span of
62 minutes is probably enough for the OP to get the point, unless
someone else REALLY feels the need to pile on.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/

--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/

On Wed, Dec 28, 2016 at 12:18 PM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:

Rebased patch of parallel-index only scan based on the latest version of
parallel index scan [1] is attached.

[1]
/messages/by-id/CAA4eK1LiNi7_Z1+PCV4y06o_v=ZdZ1UThE+W9JhthX4B8uifnA@mail.gmail.com

The link for the latest version is wrong, the correct link is:
/messages/by-id/CAA4eK1KthrAvNjmB2cWuUHz+p3ZTTtbD7o2KUw49PC8HK4r1Wg@mail.gmail.com

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello,

On applying the patch on latest master branch and running regression tests
following failure occurs.
I applied it on latest parallel index scan patches as given in the link
above.

***
/home/rahila/postgres/postgres/src/test/regress/expected/select_parallel.out
2017-01-03 14:06:29.122022780 +0530
---
/home/rahila/postgres/postgres/src/test/regress/results/select_parallel.out
2017-01-12 14:35:56.652712622 +0530
***************
*** 92,103 ****
explain (costs off)
select sum(parallel_restricted(unique1)) from tenk1
group by(parallel_restricted(unique1));
! QUERY PLAN
! ----------------------------------------------------
HashAggregate
Group Key: parallel_restricted(unique1)
! -> Index Only Scan using tenk1_unique1 on tenk1
! (3 rows)

  set force_parallel_mode=1;
  explain (costs off)
--- 92,105 ----
  explain (costs off)
    select  sum(parallel_restricted(unique1)) from tenk1
    group by(parallel_restricted(unique1));
!                             QUERY PLAN
! -------------------------------------------------------------------
   HashAggregate
     Group Key: parallel_restricted(unique1)
!    ->  Gather
!          Workers Planned: 4
!          ->  Parallel Index Only Scan using tenk1_unique1 on tenk1
! (5 rows)

IIUC, parallel operation being performed here is fine as parallel
restricted function occurs above Gather node
and just the expected output needs to be changed.

Thank you,
Rahila Syed

On Thu, Jan 12, 2017 at 5:39 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:

Hello,

On applying the patch on latest master branch and running regression tests
following failure occurs.
I applied it on latest parallel index scan patches as given in the link
above.

***
/home/rahila/postgres/postgres/src/test/regress/expected/select_parallel.out
2017-01-03 14:06:29.122022780 +0530
---
/home/rahila/postgres/postgres/src/test/regress/results/select_parallel.out
2017-01-12 14:35:56.652712622 +0530
***************
*** 92,103 ****
explain (costs off)
select sum(parallel_restricted(unique1)) from tenk1
group by(parallel_restricted(unique1));
! QUERY PLAN
! ----------------------------------------------------
HashAggregate
Group Key: parallel_restricted(unique1)
! -> Index Only Scan using tenk1_unique1 on tenk1
! (3 rows)

set force_parallel_mode=1;
explain (costs off)
--- 92,105 ----
explain (costs off)
select  sum(parallel_restricted(unique1)) from tenk1
group by(parallel_restricted(unique1));
!                             QUERY PLAN
! -------------------------------------------------------------------
HashAggregate
Group Key: parallel_restricted(unique1)
!    ->  Gather
!          Workers Planned: 4
!          ->  Parallel Index Only Scan using tenk1_unique1 on tenk1
! (5 rows)

IIUC, parallel operation being performed here is fine as parallel restricted
function occurs above Gather node
and just the expected output needs to be changed.

True, fixed it, please find the attached file for the latest version.

Thank you,
Rahila Syed

Thanks a lot for the review Rahila.
--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/

On Fri, Jan 13, 2017 at 2:19 PM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:

On Thu, Jan 12, 2017 at 5:39 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:

Hello,

On applying the patch on latest master branch and running regression tests
following failure occurs.
I applied it on latest parallel index scan patches as given in the link
above.

***
/home/rahila/postgres/postgres/src/test/regress/expected/select_parallel.out
2017-01-03 14:06:29.122022780 +0530
---
/home/rahila/postgres/postgres/src/test/regress/results/select_parallel.out
2017-01-12 14:35:56.652712622 +0530
***************
*** 92,103 ****
explain (costs off)
select sum(parallel_restricted(unique1)) from tenk1
group by(parallel_restricted(unique1));
! QUERY PLAN
! ----------------------------------------------------
HashAggregate
Group Key: parallel_restricted(unique1)
! -> Index Only Scan using tenk1_unique1 on tenk1
! (3 rows)

set force_parallel_mode=1;
explain (costs off)
--- 92,105 ----
explain (costs off)
select  sum(parallel_restricted(unique1)) from tenk1
group by(parallel_restricted(unique1));
!                             QUERY PLAN
! -------------------------------------------------------------------
HashAggregate
Group Key: parallel_restricted(unique1)
!    ->  Gather
!          Workers Planned: 4
!          ->  Parallel Index Only Scan using tenk1_unique1 on tenk1
! (5 rows)

IIUC, parallel operation being performed here is fine as parallel restricted
function occurs above Gather node
and just the expected output needs to be changed.

True, fixed it, please find the attached file for the latest version.

Please find the attached file rebased patch of parallel index-only
scan on the latest Parallel index-scan patch [1]/messages/by-id/CAA4eK1L-gb0Fum3mQN4c5PWJXNE7xs7pzwMDWsrDYLucKqvJ2A@mail.gmail.com -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/.

[1]: /messages/by-id/CAA4eK1L-gb0Fum3mQN4c5PWJXNE7xs7pzwMDWsrDYLucKqvJ2A@mail.gmail.com -- Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/
--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/

On 01/19/2017 05:37 PM, Rafia Sabih wrote:

Please find the attached file rebased patch of parallel index-only
scan on the latest Parallel index-scan patch [1].

We did some testing of this feature and written few testcases. PFA the
sql script(along with the expected .out files)

In addition we have generated the LCOV (code coverage) report and
compared the files which are changed for this.
You can see the numbers for "with_patch" V/s "with_patch+TestCases"
(.pdf file is attached)

--
regards,tushar
EnterpriseDB https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On Thu, Jan 19, 2017 at 9:07 PM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:

Please find the attached file rebased patch of parallel index-only
scan on the latest Parallel index-scan patch [1].

Moved to CF 2017-03.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Jan 19, 2017 at 7:07 AM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:

Please find the attached file rebased patch of parallel index-only
scan on the latest Parallel index-scan patch [1].

This again needs minor rebasing but basically looks fine. It's a
pretty straightforward extension of the parallel index scan work.

Please make sure that this is pgindent-clean - i.e. that when you
pgindent the files that it touches, pgindent doesn't change anything
of the same parts of the file that you've changed in the patch. Also,
I believe Amit may have made some adjustments to the logic in
nodeIndexScan.c; if so, it would be good to make sure that the
nodeIndexOnlyScan.c changes match what was done there. In particular,
he's got this:

if (reset_parallel_scan && node->iss_ScanDesc->parallel_scan)
index_parallelrescan(node->iss_ScanDesc);

And you've got this:

+               if (reset_parallel_scan)
+                       index_parallelrescan(node->ioss_ScanDesc);

There might be some other inconsistencies as well that I didn't notice
on a quick look.

--
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

I reviewed the patch. Overall it looks fine to me.

One comment,

-       if (index->amcanparallel &&
-           !index_only_scan &&
+       if ((index->amcanparallel ||
+           index_only_scan) &&

Why do we need to check for index_only_scan in the above condition. IIUC,
index->amcanparallel is necessary for
parallel index scan to be chosen. We cannot chose parallel index only scan
if index_only_scan is happening
without worrying about index->amcanparallel value. So OR-ing
index->amcanparallel with index_only_scan is probably not
correct.

Thank you,
Rahila Syed

On Thu, Feb 16, 2017 at 1:06 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Feb 16, 2017 at 1:26 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:

I reviewed the patch. Overall it looks fine to me.

One comment,

-       if (index->amcanparallel &&
-           !index_only_scan &&
+       if ((index->amcanparallel ||
+           index_only_scan) &&

Why do we need to check for index_only_scan in the above condition. IIUC,
index->amcanparallel is necessary for
parallel index scan to be chosen. We cannot chose parallel index only scan
if index_only_scan is happening
without worrying about index->amcanparallel value. So OR-ing
index->amcanparallel with index_only_scan is probably not
correct.

True, we do not need this, only removing !index_only_scan should work.

Fixed

On Thu, Feb 16, 2017 at 1:06 AM, Robert Haas <robertmhaas@gmail.com>
wrote:

This again needs minor rebasing but basically looks fine. It's a
pretty straightforward extension of the parallel index scan work.

Please make sure that this is pgindent-clean - i.e. that when you
pgindent the files that it touches, pgindent doesn't change anything
of the same parts of the file that you've changed in the patch. Also,
I believe Amit may have made some adjustments to the logic in
nodeIndexScan.c; if so, it would be good to make sure that the
nodeIndexOnlyScan.c changes match what was done there. In particular,
he's got this:

if (reset_parallel_scan && node->iss_ScanDesc->parallel_s
can)
index_parallelrescan(node->iss_ScanDesc);

And you've got this:

+               if (reset_parallel_scan)
+                       index_parallelrescan(node->ioss_ScanDesc);

Fixed.
Please find the attached patch for rebased and cleaner version.

--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/

On Thu, Feb 16, 2017 at 3:40 PM, Rafia Sabih <rafia.sabih@enterprisedb.com>
wrote:

On Thu, Feb 16, 2017 at 1:26 PM, Rahila Syed <rahilasyed90@gmail.com>
wrote:

I reviewed the patch. Overall it looks fine to me.

One comment,

-       if (index->amcanparallel &&
-           !index_only_scan &&
+       if ((index->amcanparallel ||
+           index_only_scan) &&

Why do we need to check for index_only_scan in the above condition. IIUC,
index->amcanparallel is necessary for
parallel index scan to be chosen. We cannot chose parallel index only
scan if index_only_scan is happening
without worrying about index->amcanparallel value. So OR-ing
index->amcanparallel with index_only_scan is probably not
correct.

True, we do not need this, only removing !index_only_scan should work.

Fixed

On Thu, Feb 16, 2017 at 1:06 AM, Robert Haas <robertmhaas@gmail.com>
wrote:

This again needs minor rebasing but basically looks fine. It's a
pretty straightforward extension of the parallel index scan work.

Please make sure that this is pgindent-clean - i.e. that when you
pgindent the files that it touches, pgindent doesn't change anything
of the same parts of the file that you've changed in the patch. Also,
I believe Amit may have made some adjustments to the logic in
nodeIndexScan.c; if so, it would be good to make sure that the
nodeIndexOnlyScan.c changes match what was done there. In particular,
he's got this:

if (reset_parallel_scan && node->iss_ScanDesc->parallel_s
can)
index_parallelrescan(node->iss_ScanDesc);

And you've got this:

+               if (reset_parallel_scan)
+                       index_parallelrescan(node->ioss_ScanDesc);

Fixed.
Please find the attached patch for rebased and cleaner version.

Please find the attached patch with a minor comment update.

--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/

On Thu, Feb 16, 2017 at 3:57 PM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:

On Thu, Feb 16, 2017 at 3:40 PM, Rafia Sabih <rafia.sabih@enterprisedb.com>
wrote:

Please find the attached patch for rebased and cleaner version.

Please find the attached patch with a minor comment update.

Few comments:

1.
+ * ioss_PscanLen   This is needed for parallel index scan
  * ----------------
  */
 typedef struct IndexOnlyScanState
@@ -1427,6 +1428,7 @@ typedef struct IndexOnlyScanState
  IndexScanDesc ioss_ScanDesc;
  Buffer ioss_VMBuffer;
  long ioss_HeapFetches;
+ Size ioss_PscanLen; /* This is needed for parallel index scan */

No need to mention same comment at multiple places. I think you keep
it on top of structure. The explanation could be some thing like
"size of parallel index only scan descriptor"

2.
+ node->ioss_ScanDesc->xs_want_itup = true;

I think wherever you are initializing xs_want_itup, you should
initialize ioss_VMBuffer as well. Is there a reason for not doing so?

3.
explain (costs off)
  select  sum(parallel_restricted(unique1)) from tenk1
  group by(parallel_restricted(unique1));
-                     QUERY PLAN
-----------------------------------------------------
+                            QUERY PLAN
+-------------------------------------------------------------------
  HashAggregate
    Group Key: parallel_restricted(unique1)
-   ->  Index Only Scan using tenk1_unique1 on tenk1
-(3 rows)
+   ->  Gather
+         Workers Planned: 4
+         ->  Parallel Index Only Scan using tenk1_unique1 on tenk1
+(5 rows)

It doesn't look good that you want to test parallel index only scan
for a test case that wants to test restricted function. The comments
atop test looks odd. I suggest add a separate test (both explain and
actual execution of query) for parallel index only scan as we have for
parallel plans for other queries and see if we can keep the output of
existing test same.

4.
ExecReScanIndexOnlyScan(IndexOnlyScanState *node)
{
..
+ /*
+ * if we are here to just update the scan keys, then don't reset parallel
+ * scan
+ */
+ if (node->ioss_NumRuntimeKeys != 0 && !node->ioss_RuntimeKeysReady)
+ reset_parallel_scan = false;
..
}

I think here you can update the comment to indicate that for detailed
reason refer ExecReScanIndexScan.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Feb 16, 2017 at 9:25 PM, Amit Kapila <amit.kapila16@gmail.com>
wrote:

Few comments:

1.
+ * ioss_PscanLen   This is needed for parallel index scan
* ----------------
*/
typedef struct IndexOnlyScanState
@@ -1427,6 +1428,7 @@ typedef struct IndexOnlyScanState
IndexScanDesc ioss_ScanDesc;
Buffer ioss_VMBuffer;
long ioss_HeapFetches;
+ Size ioss_PscanLen; /* This is needed for parallel index scan */

No need to mention same comment at multiple places. I think you keep
it on top of structure. The explanation could be some thing like
"size of parallel index only scan descriptor"

Fixed.

2.
+ node->ioss_ScanDesc->xs_want_itup = true;

I think wherever you are initializing xs_want_itup, you should
initialize ioss_VMBuffer as well. Is there a reason for not doing so?

Done.

3.
explain (costs off)
select  sum(parallel_restricted(unique1)) from tenk1
group by(parallel_restricted(unique1));
-                     QUERY PLAN
-----------------------------------------------------
+                            QUERY PLAN
+-------------------------------------------------------------------
HashAggregate
Group Key: parallel_restricted(unique1)
-   ->  Index Only Scan using tenk1_unique1 on tenk1
-(3 rows)
+   ->  Gather
+         Workers Planned: 4
+         ->  Parallel Index Only Scan using tenk1_unique1 on tenk1
+(5 rows)

It doesn't look good that you want to test parallel index only scan
for a test case that wants to test restricted function. The comments
atop test looks odd. I suggest add a separate test (both explain and
actual execution of query) for parallel index only scan as we have for
parallel plans for other queries and see if we can keep the output of
existing test same.

Agree, but actually the objective of this test-case is met even with this
plan. To restrict parallel index-only scan here, modification in query or
other parameters would be required. However, for the proper code-coverage
and otherwise I have added test-case for parallel index-only scan.

4.
ExecReScanIndexOnlyScan(IndexOnlyScanState *node)
{
..
+ /*
+ * if we are here to just update the scan keys, then don't reset parallel
+ * scan
+ */
+ if (node->ioss_NumRuntimeKeys != 0 && !node->ioss_RuntimeKeysReady)
+ reset_parallel_scan = false;
..
}

I think here you can update the comment to indicate that for detailed
reason refer ExecReScanIndexScan.

Done.
Please find the attached patch for the revised version.
Just an FYI, in my recent tests on TPC-H 300 scale factor, Q16 showed
improved execution time from 830 seconds to 730 seconds with this patch
when used with parallel merge-join patch [1]/messages/by-id/CAFiTN-tX3EzDw7zYvi97eNADG9PH-nmhLa24Y3uWdzy_Y4SkfQ@mail.gmail.com Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/.

[1]: /messages/by-id/CAFiTN-tX3EzDw7zYvi97eNADG9PH-nmhLa24Y3uWdzy_Y4SkfQ@mail.gmail.com Regards, Rafia Sabih EnterpriseDB: http://www.enterprisedb.com/
/messages/by-id/CAFiTN-tX3EzDw7zYvi97eNADG9PH-nmhLa24Y3uWdzy_Y4SkfQ@mail.gmail.com
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/