On Wed, Nov 30, 2016 at 11:08 AM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>>
>> patch details
>> 1. hash_support_alloc_free_v1.patch [1].
>> 2. parallel-bitmap-heap-scan-v3.patch
Few assorted comments:
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].
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.
3.
+ * Update snpashot info in heap scan descriptor.
/snpashot/snapshot
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.
5.
+ bool is_shared; /* need to build shared tbm if set*/
space is required towards the end of the comment (set */).
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"
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));
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.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com