On Thu, Feb 16, 2017 at 9:25 PM, Amit Kapila <amit.kapil...@gmail.com> wrote: > > 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" >
Fixed.
>
> 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?
>
Done.
>
>
> 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.
>
Agree, but actually the objective of this test-case is met even with this
plan. To restrict parallel index-only scan here, modification in query or
other parameters would be required. However, for the proper code-coverage
and otherwise I have added test-case for parallel index-only scan.
>
> 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.
>
Done.
Please find the attached patch for the revised version.
Just an FYI, in my recent tests on TPC-H 300 scale factor, Q16 showed
improved execution time from 830 seconds to 730 seconds with this patch
when used with parallel merge-join patch [1].
[1]
https://www.postgresql.org/message-id/CAFiTN-tX3EzDw7zYvi97eNADG9PH-nmhLa24Y3uWdzy_Y4SkfQ%40mail.gmail.com
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/
parallel_index_only_v8.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
