Parallel Index Scans

On Thu, Oct 13, 2016 at 8:48 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

As of now, the driving table for parallel query is accessed by
parallel sequential scan which limits its usage to a certain degree.
Parallelising index scans would further increase the usage of parallel
query in many more cases. This patch enables the parallelism for the
btree scans. Supporting parallel index scan for other index types
like hash, gist, spgist can be done as separate patches.

I would like to have an input on the method of selecting parallel
workers for scanning index. Currently the patch selects number of
workers based on size of index relation and the upper limit of
parallel workers is max_parallel_workers_per_gather. This is quite
similar to what we do for parallel sequential scan except for the fact
that in parallel seq. scan, we use the parallel_workers option if
provided by user during Create Table. User can provide
parallel_workers option as below:

Create Table .... With (parallel_workers = 4);

Is it desirable to have similar option for parallel index scans, if
yes then what should be the interface for same? One possible way
could be to allow user to provide it during Create Index as below:

Create Index .... With (parallel_workers = 4);

If above syntax looks sensible, then we might need to think what
should be used for parallel index build. It seems to me that parallel
tuple sort patch [1]/messages/by-id/CAM3SWZTmkOFEiCDpUNaO4n9-1xcmWP-1NXmT7h0Pu3gM2YuHvg@mail.gmail.com proposed by Peter G. is using above syntax for
getting the parallel workers input from user for parallel index
builds.

Another point which needs some thoughts is whether it is good idea to
use index relation size to calculate parallel workers for index scan.
I think ideally for index scans it should be based on number of pages
to be fetched/scanned from index.

[1]: /messages/by-id/CAM3SWZTmkOFEiCDpUNaO4n9-1xcmWP-1NXmT7h0Pu3gM2YuHvg@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

Another point which needs some thoughts is whether it is good idea to
use index relation size to calculate parallel workers for index scan.
I think ideally for index scans it should be based on number of pages
to be fetched/scanned from index.

IIUC, its not possible to know the exact number of pages scanned from an
index
in advance.
What we are essentially making parallel is the scan of the leaf pages.
So it will make sense to have the number of workers based on number of leaf
pages.
Having said that, I think it will not make much difference as compared to
existing method because
currently total index pages are used to calculate the number of workers. As
far as I understand,in large indexes, the difference between
number of leaf pages and total pages is not significant. In other words,
internal pages form a small fraction of total pages.
Also the calculation is based on log of number of pages so it will make
even lesser difference.

Thank you,
Rahila Syed

On Tue, Oct 18, 2016 at 8:38 AM, Amit Kapila <amit.kapila16@gmail.com>
wrote:

Show quoted text

On Thu, Oct 13, 2016 at 8:48 AM, Amit Kapila <amit.kapila16@gmail.com>
wrote:

As of now, the driving table for parallel query is accessed by
parallel sequential scan which limits its usage to a certain degree.
Parallelising index scans would further increase the usage of parallel
query in many more cases. This patch enables the parallelism for the
btree scans. Supporting parallel index scan for other index types
like hash, gist, spgist can be done as separate patches.

I would like to have an input on the method of selecting parallel
workers for scanning index. Currently the patch selects number of
workers based on size of index relation and the upper limit of
parallel workers is max_parallel_workers_per_gather. This is quite
similar to what we do for parallel sequential scan except for the fact
that in parallel seq. scan, we use the parallel_workers option if
provided by user during Create Table. User can provide
parallel_workers option as below:

Create Table .... With (parallel_workers = 4);

Is it desirable to have similar option for parallel index scans, if
yes then what should be the interface for same? One possible way
could be to allow user to provide it during Create Index as below:

Create Index .... With (parallel_workers = 4);

If above syntax looks sensible, then we might need to think what
should be used for parallel index build. It seems to me that parallel
tuple sort patch [1] proposed by Peter G. is using above syntax for
getting the parallel workers input from user for parallel index
builds.

Another point which needs some thoughts is whether it is good idea to
use index relation size to calculate parallel workers for index scan.
I think ideally for index scans it should be based on number of pages
to be fetched/scanned from index.

[1] - /messages/by-id/CAM3SWZTmkOFEiCDpUNaO4n9-
1xcmWP-1NXmT7h0Pu3gM2YuHvg%40mail.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

On Tue, Oct 18, 2016 at 4:08 PM, Rahila Syed <rahilasyed90@gmail.com> wrote:

Another point which needs some thoughts is whether it is good idea to
use index relation size to calculate parallel workers for index scan.
I think ideally for index scans it should be based on number of pages
to be fetched/scanned from index.

IIUC, its not possible to know the exact number of pages scanned from an
index
in advance.

We can't find the exact numbers of index pages to be scanned, but I
think we can find estimated number of pages to be fetched (refer
cost_index).

--
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 Mon, Oct 17, 2016 at 8:08 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

Create Index .... With (parallel_workers = 4);

If above syntax looks sensible, then we might need to think what
should be used for parallel index build. It seems to me that parallel
tuple sort patch [1] proposed by Peter G. is using above syntax for
getting the parallel workers input from user for parallel index
builds.

Apparently you see a similar issue with other major database systems,
where similar storage parameter things are kind of "overloaded" like
this (they are used by both index creation, and by the optimizer in
considering whether it should use a parallel index scan). That can be
a kind of a gotcha for their users, but maybe it's still worth it. In
any case, the complaints I saw about that were from users who used
parallel CREATE INDEX with the equivalent of my parallel_workers index
storage parameter, and then unexpectedly found this also forced the
use of parallel index scan. Not the other way around.

Ideally, the parallel_workers storage parameter will rarely be
necessary because the optimizer will generally do the right thing in
all case.

--
Peter Geoghegan

--
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, Oct 20, 2016 at 7:39 AM, Peter Geoghegan <pg@heroku.com> wrote:

On Mon, Oct 17, 2016 at 8:08 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

Create Index .... With (parallel_workers = 4);

If above syntax looks sensible, then we might need to think what
should be used for parallel index build. It seems to me that parallel
tuple sort patch [1] proposed by Peter G. is using above syntax for
getting the parallel workers input from user for parallel index
builds.

Apparently you see a similar issue with other major database systems,
where similar storage parameter things are kind of "overloaded" like
this (they are used by both index creation, and by the optimizer in
considering whether it should use a parallel index scan). That can be
a kind of a gotcha for their users, but maybe it's still worth it.

I have also checked and found that you are right. In SQL Server, they
are using max degree of parallelism (MAXDOP) parameter which is I
think is common for all the sql statements.

In
any case, the complaints I saw about that were from users who used
parallel CREATE INDEX with the equivalent of my parallel_workers index
storage parameter, and then unexpectedly found this also forced the
use of parallel index scan. Not the other way around.

I can understand that it can be confusing to users, so other option
could be to provide separate parameters like parallel_workers_build
and parallel_workers where first can be used for index build and
second can be used for scan. My personal opinion is to have one
parameter, so that users have one less thing to learn about
parallelism.

Ideally, the parallel_workers storage parameter will rarely be
necessary because the optimizer will generally do the right thing in
all case.

Yeah, we can choose not to provide any parameter for parallel index
scans, but some users might want to have a parameter similar to
parallel table scans, so it could be handy for them to use.

--
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 Wed, Oct 19, 2016 at 8:07 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

I have also checked and found that you are right. In SQL Server, they
are using max degree of parallelism (MAXDOP) parameter which is I
think is common for all the sql statements.

It's not just that one that does things this way, for what it's worth.

I can understand that it can be confusing to users, so other option
could be to provide separate parameters like parallel_workers_build
and parallel_workers where first can be used for index build and
second can be used for scan. My personal opinion is to have one
parameter, so that users have one less thing to learn about
parallelism.

That's my first instinct too, but I don't really have an opinion yet.

I think that this is the kind of thing where it could make sense to
take a "wait and see" approach, and then make a firm decision
immediately prior to beta. This is what we did in deciding the name of
and fine details around what ultimately became the
max_parallel_workers_per_gather GUC (plus related GUCs and storage
parameters).

--
Peter Geoghegan

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

On Wed, Oct 19, 2016 at 11:07 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

Ideally, the parallel_workers storage parameter will rarely be
necessary because the optimizer will generally do the right thing in
all case.

Yeah, we can choose not to provide any parameter for parallel index
scans, but some users might want to have a parameter similar to
parallel table scans, so it could be handy for them to use.

I think the parallel_workers reloption should override the degree of
parallelism for any sort of parallel scan on that table. Had I
intended it to apply only to sequential scans, I would have named it
differently.

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

On Thu, Oct 20, 2016 at 10:33 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Oct 19, 2016 at 11:07 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

Ideally, the parallel_workers storage parameter will rarely be
necessary because the optimizer will generally do the right thing in
all case.

Yeah, we can choose not to provide any parameter for parallel index
scans, but some users might want to have a parameter similar to
parallel table scans, so it could be handy for them to use.

I think the parallel_workers reloption should override the degree of
parallelism for any sort of parallel scan on that table. Had I
intended it to apply only to sequential scans, I would have named it
differently.

I think there is big difference of size of relation to scan between
parallel sequential scan and parallel (range) index scan which could
make it difficult for user to choose the value of this parameter. Why
do you think that the parallel_workers reloption should suffice all
type of scans for a table? I could only think of providing it based
on thinking that lesser config knobs makes life easier.

--
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 Fri, Oct 21, 2016 at 9:27 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

I think the parallel_workers reloption should override the degree of
parallelism for any sort of parallel scan on that table. Had I
intended it to apply only to sequential scans, I would have named it
differently.

I think there is big difference of size of relation to scan between
parallel sequential scan and parallel (range) index scan which could
make it difficult for user to choose the value of this parameter. Why
do you think that the parallel_workers reloption should suffice all
type of scans for a table? I could only think of providing it based
on thinking that lesser config knobs makes life easier.

Well, we could do that, but it would be fairly complicated and it
doesn't seem to me to be the right place to focus our efforts. I'd
rather try to figure out some way to make the planner smarter, because
even if users can override the number of workers on a
per-table-per-scan-type basis, they're probably still going to find
using parallel query pretty frustrating unless we make the
number-of-workers formula smarter than it is today. Anyway, even if
we do decide to add more reloptions than just parallel_degree someday,
couldn't that be left for a separate patch?

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

On Fri, Oct 21, 2016 at 10:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Oct 21, 2016 at 9:27 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

I think the parallel_workers reloption should override the degree of
parallelism for any sort of parallel scan on that table. Had I
intended it to apply only to sequential scans, I would have named it
differently.

I think there is big difference of size of relation to scan between
parallel sequential scan and parallel (range) index scan which could
make it difficult for user to choose the value of this parameter. Why
do you think that the parallel_workers reloption should suffice all
type of scans for a table? I could only think of providing it based
on thinking that lesser config knobs makes life easier.

Well, we could do that, but it would be fairly complicated and it
doesn't seem to me to be the right place to focus our efforts. I'd
rather try to figure out some way to make the planner smarter, because
even if users can override the number of workers on a
per-table-per-scan-type basis, they're probably still going to find
using parallel query pretty frustrating unless we make the
number-of-workers formula smarter than it is today. Anyway, even if
we do decide to add more reloptions than just parallel_degree someday,
couldn't that be left for a separate patch?

That makes sense to me. As of now, patch doesn't consider reloptions
for parallel index scans. So, I think we can leave it as it is and
then later as a separate patch decide whether to use reloption of
table or a separate reloption for index would be better choice.

--
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 Sat, Oct 22, 2016 at 9:07 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Oct 21, 2016 at 10:55 PM, Robert Haas <robertmhaas@gmail.com> wrote:

I have rebased the patch (parallel_index_scan_v2) based on latest
commit e8ac886c (condition variables). I have removed the usage of
ConditionVariablePrepareToSleep as that is is no longer mandatory. I
have also updated docs for wait event introduced by this patch (thanks
to Dilip for noticing it). There is no change in
parallel_index_opt_exec_support patch, but just attaching here for
easier reference.

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

On Sat, Nov 26, 2016 at 10:35 PM, Amit Kapila <amit.kapila16@gmail.com>
wrote:

On Sat, Oct 22, 2016 at 9:07 AM, Amit Kapila <amit.kapila16@gmail.com>
wrote:

On Fri, Oct 21, 2016 at 10:55 PM, Robert Haas <robertmhaas@gmail.com>

wrote:

I have rebased the patch (parallel_index_scan_v2) based on latest
commit e8ac886c (condition variables). I have removed the usage of
ConditionVariablePrepareToSleep as that is is no longer mandatory. I
have also updated docs for wait event introduced by this patch (thanks
to Dilip for noticing it). There is no change in
parallel_index_opt_exec_support patch, but just attaching here for
easier reference.

Moved to next CF with "needs review" status.

Regards,
Hari Babu
Fujitsu Australia

Hello,
On evaluating parallel index scans on TPC-H benchmark queries, I came
across some interesting results.

For scale factor 20, queries 4, 6 and 14 are giving significant performance
improvements with parallel index:
Q | Head | PI
4 | 14 | 11
6 | 27 | 9
14 | 20 | 12

To confirm that the proposed patch is scalable I tested it on 300 scale
factor, there some queries switched to bitmap index scan instead of
parallel index, but there were other queries giving significant improvement
in performance:
Q | Head | PI
4 | 207 | 168
14 | 2662 | 1576
15 | 847 | 190

All the performance numbers given above are in seconds. The experimental
setup used in this exercise is as follows:
Server parameter settings:
work_mem = 64 MB,
max_parallel_workers_per_gather = 4,
random_page_cost = seq_page_cost = 0.1 = parallel_tuple_cost,
shared_buffers = 1 GB

Logical schema: Some additional indexes were created to ensure the use of
indexes,
on lineitem table -- l_shipdate, l_returnflag, l_shipmode,
on orders table -- o_comment, o_orderdate, and
on customer table -- c_mktsegment.

Machine used: IBM Power, 4 socket machine, 512 GB RAM

Main observations about the utility and power of this patch includes
availability of appropriate indexes, giving suitable value of
random_page_cost based on the RAM and DB sizes. E.g. in these
experimentation I ensured warm cache environment, hence giving a higher
value to random_page_cost than seq_page_cost does not makes much sense and
it would inhibit the use of indexes. Also, the value of this parameter
needs to be calibrated based on the underlying hardware, there is a recent
work in this direction that gives a mechanism to do this calibration
offline, also they experimented with Postgresql parameters [1]http://pages.cs.wisc.edu/~wentaowu/papers/prediction-full.pdf.

Please find the attached file for have a look on these results in detail.
The file pi_perf_tpch.ods gives the performance numbers and the graphs for
both the scale factors. Attached zip folder gives the explain analyse
output for these queries on both head as well as with parallel index patch.

[1]: http://pages.cs.wisc.edu/~wentaowu/papers/prediction-full.pdf

On Mon, Dec 5, 2016 at 10:36 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

On Sat, Nov 26, 2016 at 10:35 PM, Amit Kapila <amit.kapila16@gmail.com>
wrote:

On Sat, Oct 22, 2016 at 9:07 AM, Amit Kapila <amit.kapila16@gmail.com>
wrote:

On Fri, Oct 21, 2016 at 10:55 PM, Robert Haas <robertmhaas@gmail.com>

wrote:

I have rebased the patch (parallel_index_scan_v2) based on latest
commit e8ac886c (condition variables). I have removed the usage of
ConditionVariablePrepareToSleep as that is is no longer mandatory. I
have also updated docs for wait event introduced by this patch (thanks
to Dilip for noticing it). There is no change in
parallel_index_opt_exec_support patch, but just attaching here for
easier reference.

Moved to next CF with "needs review" status.

Regards,
Hari Babu
Fujitsu Australia

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

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

Hi, thank you for the patch.
Results are very promising. Do you see any drawbacks of this feature or something that requires more testing?
I'm willing to oo a review. I hadn't do benchmarks yet, but I've read the patch and here are some
notes and questions about it.

I saw the discussion about parameters in the thread above. And I agree that we'd better concentrate
on the patch itself and add them later if necessary.

1. Can't we simply use "if (scan->parallel_scan != NULL)" instead of xs_temp_snap flag?

+	if (scan->xs_temp_snap)
+		UnregisterSnapshot(scan->xs_snapshot);

I must say that I'm quite new with all this parallel stuff. If you give me a link,
where to read about snapshots for parallel workers, my review will be more helpful.
Anyway, it would be great to have more comments about it in the code.

2. Don't you mind to rename 'amestimateparallelscan' to let's say 'amparallelscan_spacerequired'
or something like this? As far as I understand there is nothing to estimate, we know this size
for sure. I guess that you've chosen this name because of 'heap_parallelscan_estimate'.
But now it looks similar to 'amestimate' which refers to indexscan cost for optimizer.
That leads to the next question.

3. Are there any changes in cost estimation? I didn't find related changes in the patch.
Parallel scan is expected to be faster and optimizer definitely should know that.

4. + uint8 ps_pageStatus; /* state of scan, see below */
There is no desciption below. I'd make the comment more helpful:
/* state of scan. See possible flags values in nbtree.h */
And why do you call it pageStatus? What does it have to do with page?

5. Comment for _bt_parallel_seize() says:
"False indicates that we have reached the end of scan for
current scankeys and for that we return block number as P_NONE."

What is the reason to check (blkno == P_NONE) after checking (status == false)
in _bt_first() (see code below)? If comment is correct
we'll never reach _bt_parallel_done()

+		blkno = _bt_parallel_seize(scan, &status);
+		if (status == false)
+		{
+			BTScanPosInvalidate(so->currPos);
+			return false;
+		}
+		else if (blkno == P_NONE)
+		{
+			_bt_parallel_done(scan);
+			BTScanPosInvalidate(so->currPos);
+			return false;
+		}

6. To avoid code duplication, I would wrap this into the function

+	/* initialize moreLeft/moreRight appropriately for scan direction */
+	if (ScanDirectionIsForward(dir))
+	{
+		so->currPos.moreLeft = false;
+		so->currPos.moreRight = true;
+	}
+	else
+	{
+		so->currPos.moreLeft = true;
+		so->currPos.moreRight = false;
+	}
+	so->numKilled = 0;			/* just paranoia */
+	so->markItemIndex = -1;		/* ditto */

And after that we can also get rid of _bt_parallel_readpage() which only
bring another level of indirection to the code.

7. Just a couple of typos I've noticed:

* Below flags are used indicate the state of parallel scan.
* Below flags are used TO indicate the state of parallel scan.

* On success, release lock and pin on buffer on success.
* On success release lock and pin on buffer.

8. I didn't find a description of the feature in documentation.
Probably we need to add a paragraph to the "Parallel Query" chapter.

I will send another review of performance until the end of the week.

The new status of this patch is: Waiting on Author

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

Thanks for reviewing! A few quick thoughts from me since I write a
bunch of the design for this patch.

On Wed, Dec 21, 2016 at 10:16 AM, Anastasia Lubennikova
<lubennikovaav@gmail.com> wrote:

1. Can't we simply use "if (scan->parallel_scan != NULL)" instead of xs_temp_snap flag?

+       if (scan->xs_temp_snap)
+               UnregisterSnapshot(scan->xs_snapshot);

I must say that I'm quite new with all this parallel stuff. If you give me a link,
where to read about snapshots for parallel workers, my review will be more helpful.
Anyway, it would be great to have more comments about it in the code.

I suspect it would be better to keep those two things formally
separate, even though they may always be the same right now.

2. Don't you mind to rename 'amestimateparallelscan' to let's say 'amparallelscan_spacerequired'
or something like this? As far as I understand there is nothing to estimate, we know this size
for sure. I guess that you've chosen this name because of 'heap_parallelscan_estimate'.
But now it looks similar to 'amestimate' which refers to indexscan cost for optimizer.
That leads to the next question.

"estimate" is being used this way quite widely now, in places like
ExecParallelEstimate. So if we're going to change the terminology we
should do it broadly.

3. Are there any changes in cost estimation? I didn't find related changes in the patch.
Parallel scan is expected to be faster and optimizer definitely should know that.

Generally the way that's reflected in the optimized is by having the
parallel scan have a lower row count. See cost_seqscan() for an
example.

In general, you'll probably find a lot of parallels between this patch
and ee7ca559fcf404f9a3bd99da85c8f4ea9fbc2e92, which is probably a good
thing.

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

On Wed, Dec 21, 2016 at 8:46 PM, Anastasia Lubennikova
<lubennikovaav@gmail.com> wrote:

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

Hi, thank you for the patch.
Results are very promising. Do you see any drawbacks of this feature or something that requires more testing?

I think you can focus on the handling of array scan keys for testing.
In general, one of my colleagues has shown interest in testing this
patch and I think he has tested as well but never posted his findings.
I will request him to share his findings and what kind of tests he has
done, if any.

I'm willing to oo a review.

Thanks, that will be helpful.

I saw the discussion about parameters in the thread above. And I agree that we'd better concentrate
on the patch itself and add them later if necessary.

1. Can't we simply use "if (scan->parallel_scan != NULL)" instead of xs_temp_snap flag?

+       if (scan->xs_temp_snap)
+               UnregisterSnapshot(scan->xs_snapshot);

I agree with what Rober has told in his reply. We do same way for
heap, refer heap_endscan().

I must say that I'm quite new with all this parallel stuff. If you give me a link,
where to read about snapshots for parallel workers, my review will be more helpful.

You can read transam/README.parallel. Refer "State Sharing" portion
of README to learn more about it.

Anyway, it would be great to have more comments about it in the code.

We are sharing snapshot to ensure that reads in both master backend
and worker backend can use the same snapshot. There is no harm in
adding comments, but I think it is better to be consistent with
similar heapam code. After reading README.parallel, if you still feel
that we should add more comments in the code, then we can definitely
do that.

2. Don't you mind to rename 'amestimateparallelscan' to let's say 'amparallelscan_spacerequired'
or something like this?

Sure, I am open to other names, but IMHO, lets keep "estimate" in the
name to keep it consistent with other parallel stuff. Refer
execParallel.c to see how widely this word is used.

As far as I understand there is nothing to estimate, we know this size
for sure. I guess that you've chosen this name because of 'heap_parallelscan_estimate'.
But now it looks similar to 'amestimate' which refers to indexscan cost for optimizer.
That leads to the next question.

Do you mean 'amcostestimate'? If you want we can rename it
amparallelscanestimate to be consistent with amcostestimate.

3. Are there any changes in cost estimation?

Yes.

I didn't find related changes in the patch.
Parallel scan is expected to be faster and optimizer definitely should know that.

You can find the relavant changes in
parallel_index_opt_exec_support_v2.patch, refer cost_index().

4. + uint8 ps_pageStatus; /* state of scan, see below */
There is no desciption below. I'd make the comment more helpful:
/* state of scan. See possible flags values in nbtree.h */

makes sense. Will change.

And why do you call it pageStatus? What does it have to do with page?

During scan this tells us whether next page is available for scan.
Another option could be to name it as scanStatus, but not sure if that
is better. Do you think if we add a comment like "indicates whether
next page is available for scan" for this variable then it will be
clear?

5. Comment for _bt_parallel_seize() says:
"False indicates that we have reached the end of scan for
current scankeys and for that we return block number as P_NONE."

What is the reason to check (blkno == P_NONE) after checking (status == false)
in _bt_first() (see code below)? If comment is correct
we'll never reach _bt_parallel_done()

+               blkno = _bt_parallel_seize(scan, &status);
+               if (status == false)
+               {
+                       BTScanPosInvalidate(so->currPos);
+                       return false;
+               }
+               else if (blkno == P_NONE)
+               {
+                       _bt_parallel_done(scan);
+                       BTScanPosInvalidate(so->currPos);
+                       return false;
+               }

The first time master backend or worker hits last page (calls this
API), it will return P_NONE and after that any worker tries to fetch
next page, it will return status as false. I think we can expand a
comment to explain it clearly. Let me know, if you need more
clarification, I can explain it in detail.

6. To avoid code duplication, I would wrap this into the function

+       /* initialize moreLeft/moreRight appropriately for scan direction */
+       if (ScanDirectionIsForward(dir))
+       {
+               so->currPos.moreLeft = false;
+               so->currPos.moreRight = true;
+       }
+       else
+       {
+               so->currPos.moreLeft = true;
+               so->currPos.moreRight = false;
+       }
+       so->numKilled = 0;                      /* just paranoia */
+       so->markItemIndex = -1;         /* ditto */

Okay, I think we can write a separate function (probably inline
function) for above.

And after that we can also get rid of _bt_parallel_readpage() which only
bring another level of indirection to the code.

See, this function is responsible for multiple actions like
initializing moreLeft/moreRight positions, reading next page, dropping
the lock/pin. So replicating all these actions in the caller will
make the code in caller less readable as compared to now. Consider
this point and let me know your view on same.

7. Just a couple of typos I've noticed:

* Below flags are used indicate the state of parallel scan.
* Below flags are used TO indicate the state of parallel scan.

* On success, release lock and pin on buffer on success.
* On success release lock and pin on buffer.

Will fix.

8. I didn't find a description of the feature in documentation.
Probably we need to add a paragraph to the "Parallel Query" chapter.

Yes, I am aware of that and I think it makes sense to add it now
rather than waiting until the end.

I will send another review of performance until the end of the week.

Okay, you can refer Rafia's mail above for non-default settings she
has used in her performance tests with TPC-H.

--
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, Dec 22, 2016 at 9:49 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Dec 21, 2016 at 8:46 PM, Anastasia Lubennikova
<lubennikovaav@gmail.com> wrote:

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

Hi, thank you for the patch.
Results are very promising. Do you see any drawbacks of this feature or something that requires more testing?

I think you can focus on the handling of array scan keys for testing.
In general, one of my colleagues has shown interest in testing this
patch and I think he has tested as well but never posted his findings.
I will request him to share his findings and what kind of tests he has
done, if any.

I'm willing to oo a review.

Thanks, that will be helpful.

I saw the discussion about parameters in the thread above. And I agree that we'd better concentrate
on the patch itself and add them later if necessary.

1. Can't we simply use "if (scan->parallel_scan != NULL)" instead of xs_temp_snap flag?

+       if (scan->xs_temp_snap)
+               UnregisterSnapshot(scan->xs_snapshot);

I agree with what Rober has told in his reply.

Typo.
/Rober/Robert Haas

Thanks to Michael Paquier for noticing it and informing me offline.

--
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 12/22/2016 09:49 AM, Amit Kapila wrote:

I think you can focus on the handling of array scan keys for testing.
In general, one of my colleagues has shown interest in testing this
patch and I think he has tested as well but never posted his findings.
I will request him to share his findings and what kind of tests he has
done, if any.

Sure, We (Prabhat and I) have done some testing for this feature
internally but never published the test-scripts on this forum. PFA the
sql scripts ( along with the expected .out files) we have used for
testing for your ready reference.

In addition we had generated the LCOV (code coverage) report and
compared the files which are changed for the "Parallel index scan" patch.
You can see the numbers for "with patch" V/s "Without patch" (.pdf
file is attached)

--
regards,tushar

On 12/22/2016 01:35 PM, tushar wrote:

On 12/22/2016 09:49 AM, Amit Kapila wrote:

I think you can focus on the handling of array scan keys for testing.
In general, one of my colleagues has shown interest in testing this
patch and I think he has tested as well but never posted his findings.
I will request him to share his findings and what kind of tests he has
done, if any.

Sure, We (Prabhat and I) have done some testing for this feature
internally but never published the test-scripts on this forum. PFA the
sql scripts ( along with the expected .out files) we have used for
testing for your ready reference.

In addition we had generated the LCOV (code coverage) report and
compared the files which are changed for the "Parallel index scan" patch.
You can see the numbers for "with patch" V/s "Without patch" (.pdf
file is attached)

In addition to that, we run the sqlsmith against PG v10+PIS (parallel
index scan) patches and found a crash but that is coming on plain PG
v10 (without applying any patches) as well

postgres=# select
70 as c0,
pg_catalog.has_server_privilege(
cast(ref_0.indexdef as text),
cast(cast(coalesce((select name from pg_catalog.pg_settings
limit 1 offset 16)
,
null) as text) as text)) as c1,
pg_catalog.pg_export_snapshot() as c2,
ref_0.indexdef as c3,
ref_0.indexname as c4
from
pg_catalog.pg_indexes as ref_0
where (ref_0.tablespace = ref_0.tablespace)
or (46 = 22)
limit 103;
TRAP: FailedAssertion("!(keylen < 64)", File: "hashfunc.c", Line: 139)
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: 2016-12-23
11:19:50.627 IST [2314] LOG: server process (PID 2322) was terminated
by signal 6: Aborted
2016-12-23 11:19:50.627 IST [2314] DETAIL: Failed process was running:
select
70 as c0,
pg_catalog.has_server_privilege(
cast(ref_0.indexdef as text),
cast(cast(coalesce((select name from
pg_catalog.pg_settings limit 1 offset 16)
,
null) as text) as text)) as c1,
pg_catalog.pg_export_snapshot() as c2,
ref_0.indexdef as c3,
ref_0.indexname as c4
from
pg_catalog.pg_indexes as ref_0
where (ref_0.tablespace = ref_0.tablespace)
or (46 = 22)
limit 103;
2016-12-23 11:19:50.627 IST [2314] LOG: terminating any other active
server processes
2016-12-23 11:19:50.627 IST [2319] WARNING: terminating connection
because of crash of another server process
2016-12-23 11:19:50.627 IST [2319] DETAIL: The postmaster has commanded
this server process to roll back the current transaction and exit,
because another server process exited abnormally and possibly corrupted
shared memory.
2016-12-23 11:19:50.627 IST [2319] HINT: In a moment you should be able
to reconnect to the database and repeat your command.
2016-12-23 11:19:50.629 IST [2323] FATAL: the database system is in
recovery mode
Failed.
!> 2016-12-23 11:19:50.629 IST [2314] LOG: all server processes
terminated; reinitializing
2016-12-23 11:19:50.658 IST [2324] LOG: database system was
interrupted; last known up at 2016-12-23 11:19:47 IST
2016-12-23 11:19:50.810 IST [2324] LOG: database system was not
properly shut down; automatic recovery in progress
2016-12-23 11:19:50.812 IST [2324] LOG: invalid record length at
0/155E408: wanted 24, got 0
2016-12-23 11:19:50.812 IST [2324] LOG: redo is not required
2016-12-23 11:19:50.819 IST [2324] LOG: MultiXact member wraparound
protections are now enabled
2016-12-23 11:19:50.822 IST [2314] LOG: database system is ready to
accept connections
2016-12-23 11:19:50.822 IST [2328] LOG: autovacuum launcher started

--
regards,tushar

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