[ reviving an old thread ] On Sun, Apr 5, 2020 at 10:14 AM Tomas Vondra <[email protected]> wrote: > For the record, here is the relevant part of the Incremental Sort patch > series, updating add_partial_path and add_partial_path_precheck to also > consider startup cost. > > The changes in the first two patches are pretty straight-forward, plus > there's a proposed optimization in the precheck function to only run > compare_pathkeys if entirely necessary. I'm currently evaluating those > changes and I'll post the results to the incremental sort thread.
I rediscovered this problem while testing pg_plan_advice, and I think we should commit something to fix it. Consider the following query from the regression tests: explain (costs off) select a,b,sum(c) from t group by 1,2 order by 1,2,3 limit 1; With the settings in use at the time the plan is executed, the plan has a cost of 68.11. If you rerun the same query with enable_seqscan=off, the estimated cost drops to 65.28. This is obviously a bit of a surprising outcome, since we should be considering a subset of the original possibilities and should therefore not be able to achieve a better outcome. What happens is that the planner is deciding between Finalize GroupAggregate => Gather Merge => Partial GroupAggregate => Incremental Sort => Parallel Index Scan and GroupAggregate => Gather Merge => Incremental Sort => Parallel Index Scan. When we're computing partial paths for partially_grouped_rel, we normally construct two: one that does Incremental Sort => Parallel Index Scan and another that does Sort => Parallel Seq Scan. Because add_partial_path() doesn't consider startup costs, the second path causes the first one to be discarded. Once we add the Gather Merge node, we do consider startup costs (since it's not a partial path any more) and now the high startup cost of Sort => Parallel Seq Scan makes it lose out. Instead, we abandon the idea of two-step aggregation altogether and pick a plan that involves one step aggregation. To me, this is a pretty clear illustration that the comments that I wrote here are just wrong, the reasoning is wrong, and what we're doing doesn't make a whole lot of sense. We're throwing away paths that would turn out to be the winning option if we kept them on the basis of a specious argument that startup cost doesn't matter. So, I started working on Tomas's patches from April 5, 2020. They don't apply any more, so I had to rebase them. That resulted in a bunch of regression test failures, and some of those plan changes didn't make a whole lot of sense. At that point, I realized that when I added the disabled_nodes field to the Path structure, I failed to properly adjust the logic in add_partial_path() for the fact that we now keep path lists sorted first by number of disabled nodes and then by total cost. After fixing that, there were still two tests failing. One was the query above, where the plan improves. In the other case, a Hash Left Join became a Hash Right Join, and the reason that happened is because the two paths had exactly equivalent total costs, but the Hash Right Join has a cheaper startup cost. Arguably that's an improvement, too, but it defeated the purpose of the test case, so that will need to be adjusted. On further study, I found yet another bug, which is that add_partial_path_precheck() *also* wasn't properly adjusted to deal with the disabled_nodes field. In other words, everything that is wrong here is 100% my fault: some of it I did wrong when I added parallel query, and the rest of it I did wrong when I added disabled_nodes. So attached are a couple of patches to try to improve things. 0001 fixes the lack of a disabled_nodes test in add_partial_path. 0002 rewrites teaches add_partial_path about startup cost, and also rewrites add_partial_path_precheck to do the cost comparisons in the same manner as we do in compare_path_costs_fuzzily. This fixes both the failure of that function to consider disabled_nodes, and also the failure of that function to consider startup_cost. These are formally two separate bugs, but I do not really relish the idea of fixing them separately, because it means rewriting add_partial_path_precheck() twice, and I think the ending state will be the same as what I have here, and the intermediate state will be of no use to anyone and possibly buggy as well. The only argument I can see for trying to separate the two fixes to add_partial_path_precheck() is of one of the following two things is true: (1) We want to back-patch the disable_cost portion of the fix. This has the possibility of destabilizing plans in released branches, so I assume this is not going to be very appealing. (2) We don't actually want the changes to consider startup cost. In that case, this will definitely need to be changed. There's certainly an argument that considering the startup cost increases the number of paths we retain and therefore increases planning cost, but I feel like that argument should lose out to the counter-argument that what we're doing now is based on the fiction that the startup cost *can't* matter in the case of a partial path, which we now know is not true: James Coleman found that originally when working on Incremental Sort, and we now have a regression test query that demonstrates it. If we're going to skip considering startup cost in certain cases to reduce path explosion, it should be based on a principled design choice, which doesn't appear to be the case here. Rather, it appears to be based on vintage-2016 Robert writing some stuff that wasn't true, and then we kinda just rolled with it. -- Robert Haas EDB: http://www.enterprisedb.com
v55-0001-Fix-add_partial_path-interaction-with-disabled_n.patch
Description: Binary data
v55-0002-Consider-startup-cost-as-a-figure-of-merit-for-p.patch
Description: Binary data
