Re: Parallel bitmap heap scan

Rebased on the current head.

On Tue, Dec 13, 2016 at 12:06 PM, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> On Sat, Dec 10, 2016 at 5:36 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> Few assorted comments:
>
> Thanks for the review
>
>>
>> 1.
>> + else if (needWait)
>> + {
>> + /* Add ourself to wait queue */
>> + ConditionVariablePrepareToSleep(&pbminfo->cv);
>> + queuedSelf = true;
>> + }
>>
>> With the committed version of condition variables, you can avoid
>> calling ConditionVariablePrepareToSleep(). Refer latest parallel
>> index scan patch [1].
>
> Oh, I see, Fixed.
>>
>> 2.
>> + <entry><literal>ParallelBitmapScan</></entry>
>> + <entry>Waiting for leader to complete the TidBitmap.</entry>
>> + </row>
>> + <row>
>>
>> Isn't it better to write it as below:
>>
>> Waiting for the leader backend to form the TidBitmap.
> Done this way..
>>
>> 3.
>> + * Update snpashot info in heap scan descriptor.
>>
>> /snpashot/snapshot
> Fixed
>>
>> 4.
>> #include "utils/tqual.h"
>> -
>> +#include "miscadmin.h"
>>
>> static void bitgetpage(HeapScanDesc scan, TBMIterateResult *tbmres);
>> -
>> +static bool pbms_is_leader(ParallelBitmapInfo *pbminfo);
>>
>> TBMIterateResult *tbmres;
>> -
>> + ParallelBitmapInfo *pbminfo = node->parallel_bitmap;
>>
>> static int tbm_comparator(const void *left, const void *right);
>> -
>> +void * tbm_alloc_shared(Size size, void *arg);
>>
>> It seems line deletes at above places are spurious. Please check for
>> similar occurrences at other places in patch.
>>
> Fixed
>> 5.
>> + bool is_shared; /* need to build shared tbm if set*/
>>
>> space is required towards the end of the comment (set */).
> Fixed
>>
>> 6.
>> + /*
>> + * If we have shared TBM means we are running in parallel mode.
>> + * So directly convert dsa array to page and chunk array.
>> + */
>>
>> I think the above comment can be simplified as "For shared TBM,
>> directly convert dsa array to page and chunk array"
>
> Done this way..
>>
>> 7.
>> + dsa_entry = (void*)(((char*)dsa_entry) + sizeof(dsa_pointer));
>>
>> extra space is missing at multiple places in above line. It should be
>> written as below:
>>
>> dsa_entry = (void *)(((char *) dsa_entry) + sizeof(dsa_pointer));
> Fixed..
>
>>
>> Similar stuff needs to be taken care at other places in the patch as
>> well. I think it will be better if you run pgindent on your patch.
>
> I have run the pgindent, and taken all the changes whichever was
> applicable for added code.
>
> There are some cleanup for "hash-support-alloc-free" also, so
> attaching a new patch for this as well.
>
>
> --
> Regards,
> Dilip Kumar
> EnterpriseDB: http://www.enterprisedb.com

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com