Re: crashes due to setting max_parallel_workers=0

On 03/27/2017 01:40 PM, Rushabh Lathia wrote:
> 
> ...
> I was doing more testing with the patch and I found one more server
> crash with the patch around same area, when we forced the gather
> merge for the scan having zero rows.
> 
> create table dept ( deptno numeric, dname varchar(20);
> set parallel_tuple_cost =0;
> set parallel_setup_cost =0;
> set min_parallel_table_scan_size =0;
> set min_parallel_index_scan_size =0;
> set force_parallel_mode=regress;
> explain analyze select * from dept order by deptno;
> 
> This is because for leader we don't initialize the slot into gm_slots. So
> in case where launched worker is zero and table having zero rows, we
> end up having NULL slot into gm_slots array.
> 
> Currently gather_merge_clear_slots() clear out the tuple table slots for 
> each
> gather merge input and returns clear slot. In the patch I modified function
> gather_merge_clear_slots() to just clear out the tuple table slots and
> always return NULL when All the queues and heap us exhausted.
> 

Isn't that just another sign the code might be a bit too confusing? I 
see two main issues in the code:

1) allocating 'slots' as 'nreaders+1' elements, which seems like a good 
way to cause off-by-one errors

2) mixing objects with different life spans (slots for readers vs. slot 
for the leader) seems like a bad idea too

I wonder how much we gain by reusing the slot from the leader (I'd be 
surprised if it was at all measurable). David posted a patch reworking 
this, and significantly simplifying the GatherMerge node. Why not to 
accept that?

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services