Re: Parallel Index-only scan

From: Rafia Sabih <rafia(dot)sabih(at)enterprisedb(dot)com>
To: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Cc: Rahila Syed <rahilasyed90(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Parallel Index-only scan
Date: 2017-02-17 05:05:15
Message-ID: CAOGQiiOwC7CoVoNbHVhr+0PRa4qMqb+KV7wVMZKYK+w91xsy+g@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Thu, Feb 16, 2017 at 9:25 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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/

Attachment Content-Type Size
parallel_index_only_v8.patch application/octet-stream 13.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Prabakaran, Vaishnavi 2017-02-17 05:17:03 Re: PATCH: Batch/pipelining support for libpq
Previous Message Peter Geoghegan 2017-02-17 04:25:03 Re: Partitioning vs ON CONFLICT