Re: [HACKERS] Parallel Index-only scan

Поиск
Список
Период
Сортировка
От Rafia Sabih
Тема Re: [HACKERS] Parallel Index-only scan
Дата
Msg-id CAOGQiiOwC7CoVoNbHVhr+0PRa4qMqb+KV7wVMZKYK+w91xsy+g@mail.gmail.com
обсуждение исходный текст
Ответ на Re: [HACKERS] Parallel Index-only scan  (Amit Kapila <amit.kapila16@gmail.com>)
Ответы Re: [HACKERS] Parallel Index-only scan  (Amit Kapila <amit.kapila16@gmail.com>)
Список pgsql-hackers


On Thu, Feb 16, 2017 at 9:25 PM, Amit Kapila <amit.kapila16@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].

Regards,
Rafia Sabih
Вложения

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Peter Geoghegan
Дата:
Сообщение: Re: [HACKERS] Partitioning vs ON CONFLICT
Следующее
От: "Prabakaran, Vaishnavi"
Дата:
Сообщение: Re: [HACKERS] PATCH: Batch/pipelining support for libpq