Re: [COMMITTERS] 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: http://postgr.es/m/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(-)

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

On Fri, Mar 31, 2017 at 10:26 PM, Andres Freund <andres(at)anarazel(dot)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

On Sat, Apr 1, 2017 at 7:58 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Fri, Mar 31, 2017 at 10:26 PM, Andres Freund <andres(at)anarazel(dot)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(at)postgresql(dot)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(dot)lathia(at)gmail(dot)com>
wrote:

>
>
> On Sat, Apr 1, 2017 at 7:58 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>> On Fri, Mar 31, 2017 at 10:26 PM, Andres Freund <andres(at)anarazel(dot)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(at)postgresql(dot)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