Re: Parallel Append implementation

On Tue, Jan 17, 2017 at 2:40 PM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hi Amit,
>
> On 2016/12/23 14:21, Amit Khandekar wrote:
>> Currently an Append plan node does not execute its subplans in
>> parallel. There is no distribution of workers across its subplans. The
>> second subplan starts running only after the first subplan finishes,
>> although the individual subplans may be running parallel scans.
>>
>> Secondly, we create a partial Append path for an appendrel, but we do
>> that only if all of its member subpaths are partial paths. If one or
>> more of the subplans is a non-parallel path, there will be only a
>> non-parallel Append. So whatever node is sitting on top of Append is
>> not going to do a parallel plan; for example, a select count(*) won't
>> divide it into partial aggregates if the underlying Append is not
>> partial.
>>
>> The attached patch removes both of the above restrictions. There has
>> already been a mail thread [1] that discusses an approach suggested by
>> Robert Haas for implementing this feature. This patch uses this same
>> approach.
>
> I was looking at the executor portion of this patch and I noticed that in
> exec_append_initialize_next():
>
> if (appendstate->as_padesc)
> return parallel_append_next(appendstate);
>
> /*
> * Not parallel-aware. Fine, just go on to the next subplan in the
> * appropriate direction.
> */
> if (ScanDirectionIsForward(appendstate->ps.state->es_direction))
> appendstate->as_whichplan++;
> else
> appendstate->as_whichplan--;
>
> which seems to mean that executing Append in parallel mode disregards the
> scan direction. I am not immediately sure what implications that has, so
> I checked what heap scan does when executing in parallel mode, and found
> this in heapgettup():
>
> else if (backward)
> {
> /* backward parallel scan not supported */
> Assert(scan->rs_parallel == NULL);
>
> Perhaps, AppendState.as_padesc would not have been set if scan direction
> is backward, because parallel mode would be disabled for the whole query
> in that case (PlannerGlobal.parallelModeOK = false). Maybe add an
> Assert() similar to one in heapgettup().

There have been some reviews, but the patch has not been updated in
two weeks. Marking as "returned with feedback".
--
Michael