Re: pgsql: Support Parallel Append plan nodes. - Mailing list pgsql-committers

On 7 December 2017 at 12:32, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
> On 6 December 2017 at 20:59, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Wed, Dec 6, 2017 at 5:01 AM, Amit Khandekar <amitdkhan.pg@gmail.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