Fix incorrect logic for choosing the next Parallel Append subplan

In 499be013de support for pruning unneeded Append subnodes was added.
The logic in that commit was not correctly checking if the next subplan
was in fact a valid subplan. This could cause parallel workers processes
to be given a subplan to work on which didn't require any work.

Per code review following an otherwise unexplained regression failure in
buildfarm member Pademelon.  (We haven't been able to reproduce the
failure, so this is a bit of a blind fix in terms of whether it'll
actually fix it; but it is a clear bug nonetheless).

In passing, also add a comment to explain what first_partial_plan means.

Author: David Rowley
Discussion: https://postgr.es/m/CAKJS1f_E5r05hHUVG3UmCQJ49DGKKHtN=SHybD44LdzBn+CJng@mail.gmail.com
This commit is contained in:
Alvaro Herrera 2018-04-09 17:23:49 -03:00
parent d7754822c5
commit 468abb8f7a
2 changed files with 43 additions and 12 deletions

View File

@ -547,6 +547,11 @@ choose_next_subplan_for_leader(AppendState *node)
LWLockRelease(&pstate->pa_lock);
return false;
}
/*
* We needn't pay attention to as_valid_subplans here as all invalid
* plans have been marked as finished.
*/
node->as_whichplan--;
}
@ -612,18 +617,28 @@ choose_next_subplan_for_worker(AppendState *node)
/* Save the plan from which we are starting the search. */
node->as_whichplan = pstate->pa_next_plan;
/* Loop until we find a subplan to execute. */
/* Loop until we find a valid subplan to execute. */
while (pstate->pa_finished[pstate->pa_next_plan])
{
if (pstate->pa_next_plan < node->as_nplans - 1)
int nextplan;
nextplan = bms_next_member(node->as_valid_subplans,
pstate->pa_next_plan);
if (nextplan >= 0)
{
/* Advance to next plan. */
pstate->pa_next_plan++;
/* Advance to the next valid plan. */
pstate->pa_next_plan = nextplan;
}
else if (node->as_whichplan > append->first_partial_plan)
{
/* Loop back to first partial plan. */
pstate->pa_next_plan = append->first_partial_plan;
/*
* Try looping back to the first valid partial plan, if there is
* one. If there isn't, arrange to bail out below.
*/
nextplan = bms_next_member(node->as_valid_subplans,
append->first_partial_plan - 1);
pstate->pa_next_plan =
nextplan < 0 ? node->as_whichplan : nextplan;
}
else
{
@ -644,16 +659,27 @@ choose_next_subplan_for_worker(AppendState *node)
}
/* Pick the plan we found, and advance pa_next_plan one more time. */
node->as_whichplan = pstate->pa_next_plan++;
if (pstate->pa_next_plan >= node->as_nplans)
node->as_whichplan = pstate->pa_next_plan;
pstate->pa_next_plan = bms_next_member(node->as_valid_subplans,
pstate->pa_next_plan);
/*
* If there are no more valid plans then try setting the next plan to the
* first valid partial plan.
*/
if (pstate->pa_next_plan < 0)
{
if (append->first_partial_plan < node->as_nplans)
pstate->pa_next_plan = append->first_partial_plan;
int nextplan = bms_next_member(node->as_valid_subplans,
append->first_partial_plan - 1);
if (nextplan >= 0)
pstate->pa_next_plan = nextplan;
else
{
/*
* We have only non-partial plans, and we already chose the last
* one; so arrange for the other workers to immediately bail out.
* There are no valid partial plans, and we already chose the last
* non-partial plan; so flag that there's nothing more for our
* fellow workers to do.
*/
pstate->pa_next_plan = INVALID_SUBPLAN_INDEX;
}

View File

@ -255,6 +255,11 @@ typedef struct Append
/* RT indexes of non-leaf tables in a partition tree */
List *partitioned_rels;
List *appendplans;
/*
* All 'appendplans' preceding this index are non-partial plans. All
* 'appendplans' from this index onwards are partial plans.
*/
int first_partial_plan;
/*