Re: pgsql: Support Parallel Append plan nodes.

On 7 December 2017 at 12:32, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
> On 6 December 2017 at 20:59, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Wed, Dec 6, 2017 at 5:01 AM, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
>>>> In attached revised patch, just added some comments in the changes that you did.
>>
>>> Committed, thanks.
>>
>> While this patch (looks like it) fixes the logic error, it does nothing
>> for the problem that the committed test cases don't actually exercise
>> any of this code on most machines -- certainly not whichever one is
>> producing the code coverage report:
>> https://coverage.postgresql.org/src/backend/executor/nodeAppend.c.gcov.html
>>
>> Can we do something with Andres' suggestion of running these test cases
>> under parallel_leader_participation = off?
>>
>> regards, tom lane
>
> Yes, I am planning to send a patch that makes all those
> Parallel-Append test cases run once with parallel_leader_participation
> "on" and then run again with the guc "off" . Thanks.
>

Attached is a patch that adds the above test cases. This allows
coverage for the function choose_next_subplan_for_worker().

The code to advance pa_next_plan is there in the for-loop (place_1)
and again below the for loop (place_2). At both these places, the
logic involves wrapping around to the first (partial) plan. The code
coverage exists for this wrap-around logic at place_2, but not at
place_1 (i.e. in the for loop) :

470 : /* If all the plans are already done, we have nothing to do */
471 72 : if (pstate->pa_next_plan == INVALID_SUBPLAN_INDEX)
472 : {
473 32 : LWLockRelease(&pstate->pa_lock);
474 32 : return false;
475 : }
476 :
477 : /* Loop until we find a subplan to execute. */
478 92 : while (pstate->pa_finished[pstate->pa_next_plan])
479 : {
480 16 : if (pstate->pa_next_plan < node->as_nplans - 1)
481 : {
482 : /* Advance to next plan. */
483 16 : pstate->pa_next_plan++;
484 : }
485 0 : else if (append->first_partial_plan < node->as_nplans)
486 : {
487 : /* Loop back to first partial plan. */
488 0 : pstate->pa_next_plan = append->first_partial_plan;
489 : }
490 : else
491 : {
492 : /* At last plan, no partial plans, arrange to bail out. */
493 0 : pstate->pa_next_plan = node->as_whichplan;
494 : }
495 :
496 16 : if (pstate->pa_next_plan == node->as_whichplan)
497 : {
498 : /* We've tried everything! */
499 4 : pstate->pa_next_plan = INVALID_SUBPLAN_INDEX;
500 4 : LWLockRelease(&pstate->pa_lock);
501 4 : return false;
502 : }
503 : }

I have not managed to make the regression test cases execute the above
not-covered case. I could do that with my local test that I have, but
that test has lots of data, so it is slow, and not suitable for
regression. In select_parallel.sql, by the time a worker w1 gets into
the function choose_next_subplan_for_worker(), an earlier worker w2
must have already wrapped around the pa_next_plan at place_2, so this
worker doesn't get a chance to hit place_1. It's a timing issue. I
will see if I can solve this.

--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company