pgsql: Avoid GatherMerge crash when there are no workers.

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

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

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