On Thu, Feb 16, 2017 at 3:57 PM, Rafia Sabih <rafia.sa...@enterprisedb.com> wrote: > > > On Thu, Feb 16, 2017 at 3:40 PM, Rafia Sabih <rafia.sa...@enterprisedb.com> > wrote: >> >> >> Please find the attached patch for rebased and cleaner version. >> > Please find the attached patch with a minor comment update. >
Few comments: 1. + * ioss_PscanLen This is needed for parallel index scan * ---------------- */ typedef struct IndexOnlyScanState @@ -1427,6 +1428,7 @@ typedef struct IndexOnlyScanState IndexScanDesc ioss_ScanDesc; Buffer ioss_VMBuffer; long ioss_HeapFetches; + Size ioss_PscanLen; /* This is needed for parallel index scan */ No need to mention same comment at multiple places. I think you keep it on top of structure. The explanation could be some thing like "size of parallel index only scan descriptor" 2. + node->ioss_ScanDesc->xs_want_itup = true; I think wherever you are initializing xs_want_itup, you should initialize ioss_VMBuffer as well. Is there a reason for not doing so? 3. explain (costs off) select sum(parallel_restricted(unique1)) from tenk1 group by(parallel_restricted(unique1)); - QUERY PLAN ----------------------------------------------------- + QUERY PLAN +------------------------------------------------------------------- HashAggregate Group Key: parallel_restricted(unique1) - -> Index Only Scan using tenk1_unique1 on tenk1 -(3 rows) + -> Gather + Workers Planned: 4 + -> Parallel Index Only Scan using tenk1_unique1 on tenk1 +(5 rows) It doesn't look good that you want to test parallel index only scan for a test case that wants to test restricted function. The comments atop test looks odd. I suggest add a separate test (both explain and actual execution of query) for parallel index only scan as we have for parallel plans for other queries and see if we can keep the output of existing test same. 4. ExecReScanIndexOnlyScan(IndexOnlyScanState *node) { .. + /* + * if we are here to just update the scan keys, then don't reset parallel + * scan + */ + if (node->ioss_NumRuntimeKeys != 0 && !node->ioss_RuntimeKeysReady) + reset_parallel_scan = false; .. } I think here you can update the comment to indicate that for detailed reason refer ExecReScanIndexScan. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers