On 7 December 2017 at 12:32, Amit Khandekar <[email protected]> wrote: > On 6 December 2017 at 20:59, Tom Lane <[email protected]> wrote: >> Robert Haas <[email protected]> writes: >>> On Wed, Dec 6, 2017 at 5:01 AM, Amit Khandekar <[email protected]> >>> 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
test_with_noleader.patch
Description: Binary data
