pgsql: Avoid GatherMerge crash when there are no workers.
Avoid GatherMerge crash when there are no workers.
It's unnecessary to return an actual slot when we have no tuple.
We can just return NULL, which avoids the risk of indexing into an
array that might not contain any elements.
Rushabh Lathia, per a report from Tomas Vondra
Discussion: /messages/by-id/6ecd6f17-0dcf-1de7-ded8-0de7db1ddc88@2ndquadrant.com
Branch
------
master
Details
-------
http://git.postgresql.org/pg/commitdiff/25dc142a49c60c3107480c487cd8444dc83f9bdf
Modified Files
--------------
src/backend/executor/nodeGatherMerge.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Hi,
On 2017-04-01 01:22:14 +0000, Robert Haas wrote:
Avoid GatherMerge crash when there are no workers.
I think the gather merge code needs a bit more test coverage (sorry to
make this a larger theme today). As shown by
https://coverage.postgresql.org/src/backend/executor/nodeGatherMerge.c.gcov.html
we don't actually merge anything (heap_compare_slots is not exercised).
I btw also wonder if it's good that we have a nearly identical copy of
heap_compare_slots and a bunch of the calling code in both
nodeMergeAppend.c and nodeGatherMerge.c. On the other hand, it's not
heavily envolving code.
- Andres
--
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, Mar 31, 2017 at 10:26 PM, Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2017-04-01 01:22:14 +0000, Robert Haas wrote:
Avoid GatherMerge crash when there are no workers.
I think the gather merge code needs a bit more test coverage (sorry to
make this a larger theme today). As shown by
https://coverage.postgresql.org/src/backend/executor/nodeGatherMerge.c.gcov.html
we don't actually merge anything (heap_compare_slots is not exercised).
Sounds reasonable.
I btw also wonder if it's good that we have a nearly identical copy of
heap_compare_slots and a bunch of the calling code in both
nodeMergeAppend.c and nodeGatherMerge.c. On the other hand, it's not
heavily envolving code.
Yeah, I don't know. We could alternatively try to move that to some
common location and merge the two implementations. I'm not sure
exactly where, though.
--
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 Sat, Apr 1, 2017 at 7:58 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Fri, Mar 31, 2017 at 10:26 PM, Andres Freund <andres@anarazel.de>
wrote:Hi,
On 2017-04-01 01:22:14 +0000, Robert Haas wrote:
Avoid GatherMerge crash when there are no workers.
I think the gather merge code needs a bit more test coverage (sorry to
make this a larger theme today). As shown by
https://coverage.postgresql.org/src/backend/executor/nodeGatherMerge.c.gcov.html
we don't actually merge anything (heap_compare_slots is not exercised).
Sounds reasonable.
I am working on this and will submit the patch soon.
I btw also wonder if it's good that we have a nearly identical copy of
heap_compare_slots and a bunch of the calling code in both
nodeMergeAppend.c and nodeGatherMerge.c. On the other hand, it's not
heavily envolving code.Yeah, I don't know. We could alternatively try to move that to some
common location and merge the two implementations. I'm not sure
exactly where, though.--
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
--
Rushabh Lathia
Hi,
On Mon, Apr 3, 2017 at 10:56 AM, Rushabh Lathia <rushabh.lathia@gmail.com>
wrote:
On Sat, Apr 1, 2017 at 7:58 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Fri, Mar 31, 2017 at 10:26 PM, Andres Freund <andres@anarazel.de>
wrote:Hi,
On 2017-04-01 01:22:14 +0000, Robert Haas wrote:
Avoid GatherMerge crash when there are no workers.
I think the gather merge code needs a bit more test coverage (sorry to
make this a larger theme today). As shown by
https://coverage.postgresql.org/src/backend/executor/nodeGatherMerge.c.gcov.html
we don't actually merge anything (heap_compare_slots is not exercised).
Sounds reasonable.
I am working on this and will submit the patch soon.
On my local environment I was getting coverage for the heap_compare_slots
with
existing test. But it seems like due to low number of record fetch only
leader get
evolved to the fetching tuples in the shared report.
I modified the current test coverage for the Gather Merge to add more rows
to be
fetch by the GatherMerge node to make sure that it do coverage for
heap_compare_slots. Also added test for the zero worker.
PFA patch as well as LCOV report for the nodeGatherMerge.c.
I btw also wonder if it's good that we have a nearly identical copy of
heap_compare_slots and a bunch of the calling code in both
nodeMergeAppend.c and nodeGatherMerge.c. On the other hand, it's not
heavily envolving code.Yeah, I don't know. We could alternatively try to move that to some
common location and merge the two implementations. I'm not sure
exactly where, though.--
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--
Rushabh Lathia
--
Rushabh Lathia
www.EnterpriseDB.com
Attachments:
gather_merge_test_coverage.patchtext/x-patch; charset=US-ASCII; name=gather_merge_test_coverage.patchDownload
diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out
index 038a62e..8395f4b 100644
--- a/src/test/regress/expected/select_parallel.out
+++ b/src/test/regress/expected/select_parallel.out
@@ -213,32 +213,73 @@ select count(*) from tenk1, tenk2 where tenk1.unique1 = tenk2.unique1;
reset enable_hashjoin;
reset enable_nestloop;
---test gather merge
-set enable_hashagg to off;
+-- test gather merge
+set enable_hashagg = false;
explain (costs off)
- select string4, count((unique2)) from tenk1 group by string4 order by string4;
+ select count(*) from tenk1 group by twenty;
QUERY PLAN
----------------------------------------------------
Finalize GroupAggregate
- Group Key: string4
+ Group Key: twenty
-> Gather Merge
Workers Planned: 4
-> Partial GroupAggregate
- Group Key: string4
+ Group Key: twenty
-> Sort
- Sort Key: string4
+ Sort Key: twenty
-> Parallel Seq Scan on tenk1
(9 rows)
-select string4, count((unique2)) from tenk1 group by string4 order by string4;
- string4 | count
----------+-------
- AAAAxx | 2500
- HHHHxx | 2500
- OOOOxx | 2500
- VVVVxx | 2500
-(4 rows)
+select count(*) from tenk1 group by twenty;
+ count
+-------
+ 500
+ 500
+ 500
+ 500
+ 500
+ 500
+ 500
+ 500
+ 500
+ 500
+ 500
+ 500
+ 500
+ 500
+ 500
+ 500
+ 500
+ 500
+ 500
+ 500
+(20 rows)
+
+-- gather merge test with 0 worker
+set max_parallel_workers = 0;
+explain (costs off)
+ select string4 from tenk1 order by string4 limit 5;
+ QUERY PLAN
+----------------------------------------------
+ Limit
+ -> Gather Merge
+ Workers Planned: 4
+ -> Sort
+ Sort Key: string4
+ -> Parallel Seq Scan on tenk1
+(6 rows)
+
+select string4 from tenk1 order by string4 limit 5;
+ string4
+---------
+ AAAAxx
+ AAAAxx
+ AAAAxx
+ AAAAxx
+ AAAAxx
+(5 rows)
+reset max_parallel_workers;
reset enable_hashagg;
set force_parallel_mode=1;
explain (costs off)
diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql
index 9311a77..d0d1290 100644
--- a/src/test/regress/sql/select_parallel.sql
+++ b/src/test/regress/sql/select_parallel.sql
@@ -85,14 +85,20 @@ select count(*) from tenk1, tenk2 where tenk1.unique1 = tenk2.unique1;
reset enable_hashjoin;
reset enable_nestloop;
---test gather merge
-set enable_hashagg to off;
+-- test gather merge
+set enable_hashagg = false;
explain (costs off)
- select string4, count((unique2)) from tenk1 group by string4 order by string4;
+ select count(*) from tenk1 group by twenty;
-select string4, count((unique2)) from tenk1 group by string4 order by string4;
+select count(*) from tenk1 group by twenty;
+-- gather merge test with 0 worker
+set max_parallel_workers = 0;
+explain (costs off)
+ select string4 from tenk1 order by string4 limit 5;
+select string4 from tenk1 order by string4 limit 5;
+reset max_parallel_workers;
reset enable_hashagg;
set force_parallel_mode=1;
Hi,
On 2017-04-03 12:56:36 +0530, Rushabh Lathia wrote:
On my local environment I was getting coverage for the heap_compare_slots
with
existing test. But it seems like due to low number of record fetch only
leader get
evolved to the fetching tuples in the shared report.I modified the current test coverage for the Gather Merge to add more rows
to be
fetch by the GatherMerge node to make sure that it do coverage for
heap_compare_slots. Also added test for the zero worker.PFA patch as well as LCOV report for the nodeGatherMerge.c.
Pushed, thanks!
- Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers