Re: Parallel Index-only scan

On Thu, Feb 16, 2017 at 3:57 PM, Rafia Sabih
<rafia(dot)sabih(at)enterprisedb(dot)com> wrote:
>
>
> On Thu, Feb 16, 2017 at 3:40 PM, Rafia Sabih <rafia(dot)sabih(at)enterprisedb(dot)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