Re: Parallel bitmap heap scan

On Sat, Dec 10, 2016 at 5:36 PM, Amit Kapila <amit.kapila16@gmail.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