On Tue, Mar 7, 2017 at 11:18 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Mar 7, 2017 at 10:22 AM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>>> I also think that commit didn't intend to change the behavior,
>>> however, the point is how sensible is it to keep such behavior after
>>> Parallel Append. I am not sure if this is the right time to consider
>>> it or shall we wait till Parallel Append is committed.
>>>
>>>> I think the problem here is that compute_parallel_worker() thinks it
>>>> can use 0 as a sentinel value that means "ignore this", but it can't
>>>> really, because a heap can actually contain 0 pages. Here's a patch
>>>> which does the following:
>>>>
>>>
>>> - if (heap_pages < (BlockNumber) min_parallel_table_scan_size &&
>>> - index_pages < (BlockNumber) min_parallel_index_scan_size &&
>>> - rel->reloptkind == RELOPT_BASEREL)
>>> + if (rel->reloptkind == RELOPT_BASEREL &&
>>> + ((heap_pages >= 0 && heap_pages < min_parallel_table_scan_size) ||
>>> + (index_pages >= 0 && index_pages < min_parallel_index_scan_size)))
>>> return 0;
>>>
>>> The purpose of considering both heap and index pages () for
>>> min_parallel_* is that even if one of them is bigger than threshold
>>> then we should try parallelism.
>>
>> Yes, But, this is only true for normal tables not for partitioned or
>> inheritance tables. I think for partition table, even if one heap page
>> exist, we go for parallelism.
>>
>> This could be helpful for parallel
>>> index scans where we consider parallel workers based on both heap and
>>> index pages. Is there a reason for you to change that condition as
>>> that is not even related to the problem being discussed?
>>>
>>
>> I think he has changed to allow parallelism for inheritance or partition
>> tables. For normal tables, it won't be touched until the below if-condition
>> is satisfied.
>>
>> if (heap_pages < (BlockNumber) min_parallel_table_scan_size &&
>> index_pages < (BlockNumber) min_parallel_index_scan_size &&
>> rel->reloptkind == RELOPT_BASEREL)
>> return 0;
>>
>
> AFAICS, the patch has changed the if-condition you are quoting.
>
Yes, It has slightly changed the if condition or I would say it has
corrected the if condition. The reason being, with new if condition in
patch, we first check if reloptkind is of BASEREL type or not before
checking if there are enough heap pages or index pages that is good to
go for parallelism. In case if it is partition table 'rel->reloptkind
== RELOPT_BASEREL' will fail hence, further conditions won't be
checked.
I think the most important thing that the patch fixes is passing
index_pages as '-1' to compute_parallel_worker() as
'min_parallel_index_scan_size' can be set to zero by the user.
--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com
