Re: [HACKERS] [COMMITTERS] pgsql: Avoid GatherMerge crash when there are no workers.

2017-08-14 Thread Andres Freund
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


Re: [HACKERS] [COMMITTERS] pgsql: Avoid GatherMerge crash when there are no workers.

2017-04-03 Thread Rushabh Lathia
Hi,

On Mon, Apr 3, 2017 at 10:56 AM, Rushabh Lathia 
wrote:

>
>
> On Sat, Apr 1, 2017 at 7:58 PM, Robert Haas  wrote:
>
>> On Fri, Mar 31, 2017 at 10:26 PM, Andres Freund 
>> wrote:
>> > Hi,
>> >
>> > On 2017-04-01 01:22:14 +, 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
Title: LCOV - PostgreSQL - src/backend/executor/nodeGatherMerge.c








  
LCOV - code coverage report



  

  
Current view:
top level - src/backend/executor - nodeGatherMerge.c (source / functions)


Hit
Total
Coverage
  
  
Test:
PostgreSQL

Lines:
200
208
96.2 %
  
  
Date:
2017-04-03

Functions:
12
13
92.3 %
  
  
Legend:
Lines:
hit
not hit


  
  

  



  

  

  


  
  Line dataSource code

   1 : /*-
   2 :  *
   3 :  * nodeGatherMerge.c
   4 :  *  Scan a plan in multiple workers, and do order-preserving merge.
   5 :  *
   6 :  * Portions Copyright (c) 1996-2017, PostgreSQL Global Development Group
   7 :  * Portions Copyright (c) 1994, Regents of the University of California
   8 :  *
   9 :  * IDENTIFICATION
  10 :  *src/backend/executor/nodeGatherMerge.c
  11 :  *
  12 :  *-
  13 :  */
  14 : 
  15 : #include "postgres.h"
  16 : 
  17 : #include "access/relscan.h"
  18 : #include "access/xact.h"
  19 : #include "executor/execdebug.h"
  20 : #include "executor/execParallel.h"
  21 : #include "executor/nodeGatherMerge.h"
  22 : #include "executor/nodeSubplan.h"
  23 : #include "executor/tqueue.h"
  24 : #include "lib/binaryheap.h"
  25 : #include "miscadmin.h"
  26 : #include "utils/memutils.h"
  27 : #include "utils/rel.h"
  28 : 
  29 : /*
  30 :  * Tuple array for each worker
  31 :  */
  32 : typedef struct GMReaderTupleBuffer
  33 : {
  34 : HeapTuple  *tuple;
  35 : int readCounter;
  36 : int nTuples;
  37 : booldone;
  38 : }   GMReaderTupleBuffer;
  39 : 
  40 : /*
  41 :  * When we read tuples from workers, it's a good idea to read several at once
  42 :  * for efficiency when possible: 

Re: [HACKERS] [COMMITTERS] pgsql: Avoid GatherMerge crash when there are no workers.

2017-04-02 Thread Rushabh Lathia
On Sat, Apr 1, 2017 at 7:58 PM, Robert Haas  wrote:

> On Fri, Mar 31, 2017 at 10:26 PM, Andres Freund 
> wrote:
> > Hi,
> >
> > On 2017-04-01 01:22:14 +, 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


Re: [HACKERS] [COMMITTERS] pgsql: Avoid GatherMerge crash when there are no workers.

2017-04-01 Thread Robert Haas
On Fri, Mar 31, 2017 at 10:26 PM, Andres Freund  wrote:
> Hi,
>
> On 2017-04-01 01:22:14 +, 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