From a497288426699e5529776fe60cc72ed3d0af98d1 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Sun, 28 Jul 2019 15:55:54 +0200 Subject: [PATCH 1/4] Consider low startup cost when adding partial path 45be99f8cd5d606086e0a458c9c72910ba8a613d added `add_partial_path` with the comment: > Neither do we need to consider startup costs: > parallelism is only used for plans that will be run to completion. > Therefore, this routine is much simpler than add_path: it needs to > consider only pathkeys and total cost. I'm not entirely sure if that is still true or not--I can't easily come up with a scenario in which it's not, but I also can't come up with an inherent reason why such a scenario cannot exist. Regardless, the in-progress incremental sort patch uncovered a new case where it definitely no longer holds, and, as a result a higher cost plan ends up being chosen because a low startup cost partial path is ignored in favor of a lower total cost partial path and a limit is a applied on top of that which would normal favor the lower startup cost plan. --- src/backend/optimizer/util/pathnode.c | 65 +++++++++++++-------------- 1 file changed, 31 insertions(+), 34 deletions(-) diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c index 8ba8122ee2..b570bfd3be 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/src/backend/optimizer/util/pathnode.c @@ -733,10 +733,11 @@ add_path_precheck(RelOptInfo *parent_rel, * * Because we don't consider parameterized paths here, we also don't * need to consider the row counts as a measure of quality: every path will - * produce the same number of rows. Neither do we need to consider startup - * costs: parallelism is only used for plans that will be run to completion. - * Therefore, this routine is much simpler than add_path: it needs to - * consider only pathkeys and total cost. + * produce the same number of rows. It may however matter how much the + * path ordering matches the final ordering, needed by upper parts of the + * plan. Because that will affect how expensive the incremental sort is, + * we need to consider both the total and startup path, in addition to + * pathkeys. * * As with add_path, we pfree paths that are found to be dominated by * another partial path; this requires that there be no other references to @@ -774,44 +775,40 @@ add_partial_path(RelOptInfo *parent_rel, Path *new_path) /* Compare pathkeys. */ keyscmp = compare_pathkeys(new_path->pathkeys, old_path->pathkeys); - /* Unless pathkeys are incompatible, keep just one of the two paths. */ + /* + * Unless pathkeys are incompatible, see if one of the paths dominates + * the other (both in startup and total cost). It may happen that one + * path has lower startup cost, the other has lower total cost. + * + * XXX Perhaps we could do this only when incremental sort is enabled, + * and use the simpler version (comparing just total cost) otherwise? + */ if (keyscmp != PATHKEYS_DIFFERENT) { - if (new_path->total_cost > old_path->total_cost * STD_FUZZ_FACTOR) - { - /* New path costs more; keep it only if pathkeys are better. */ - if (keyscmp != PATHKEYS_BETTER1) - accept_new = false; - } - else if (old_path->total_cost > new_path->total_cost - * STD_FUZZ_FACTOR) + PathCostComparison costcmp; + + /* + * Do a fuzzy cost comparison with standard fuzziness limit. + */ + costcmp = compare_path_costs_fuzzily(new_path, old_path, + STD_FUZZ_FACTOR); + + if (costcmp == COSTS_BETTER1) { - /* Old path costs more; keep it only if pathkeys are better. */ - if (keyscmp != PATHKEYS_BETTER2) + if (keyscmp == PATHKEYS_BETTER1) remove_old = true; } - else if (keyscmp == PATHKEYS_BETTER1) + else if (costcmp == COSTS_BETTER2) { - /* Costs are about the same, new path has better pathkeys. */ - remove_old = true; - } - else if (keyscmp == PATHKEYS_BETTER2) - { - /* Costs are about the same, old path has better pathkeys. */ - accept_new = false; - } - else if (old_path->total_cost > new_path->total_cost * 1.0000000001) - { - /* Pathkeys are the same, and the old path costs more. */ - remove_old = true; + if (keyscmp == PATHKEYS_BETTER2) + accept_new = false; } - else + else if (costcmp == COSTS_EQUAL) { - /* - * Pathkeys are the same, and new path isn't materially - * cheaper. - */ - accept_new = false; + if (keyscmp == PATHKEYS_BETTER1) + remove_old = true; + else if (keyscmp == PATHKEYS_BETTER2) + accept_new = false; } } -- 2.21.1