OK, here's a new patch version. I earlier committed the refactoring to avoid using disable_cost to force WHERE CURRENT OF to be implemented by a TID scan. In this version, I've dropped everything related to reworking enable_indexscan or any other enable_* GUC. Hence, this version of the patch set just focuses on adding the count of disabled nodes and removing the use of disable_cost. In addition to dropping the controversial patches, I've also found and squashed a few bugs in this version.
Behavior: With the patch, whenever an enable_* GUC would cause disable_cost to be added, disabled_nodes is incremented instead. There is one remaining use of disable_cost which is not triggered by an enable_* GUC but by the desire to avoid plans that we think will overflow work_mem. I welcome thoughts on what to do about that case; for now, I do nothing. As before, 0001 adds the disabled_nodes field to paths and 0002 adds it to plans. I think we could plausibly commit only 0001, both patches separately, or both patches squashed. Notes: - I favor committing both patches. Tom stated that he didn't think that we needed to show anything related to disabled nodes, and that could be true. However, today, you can tell which nodes are disabled as long as you print out the costs; if we don't propagate disabled nodes into the plan and print them out, that will no longer be possible. I found working on the patches that it was really hard to debug the patch set without this, so my guess is that we'll find not having it pretty annoying, but we can also just commit 0001 for starters and see how long it takes for the lack of 0002 to become annoying. If the answer is "infinite time," that's cool; if it isn't, we can reconsider committing 0002. - If we do commit 0002, I think it's a good idea to have the number of disabled nodes displayed even with COSTS OFF, because it's stable, and it's pretty useful to be able to see this in the regression output. I have found while working on this that I often need to adjust the .sql files to say EXPLAIN (COSTS ON) instead of EXPLAIN (COSTS OFF) in order to understand what's happening. Right now, there's no real alternative because costs aren't stable, but disabled-node counts should be stable, so I feel this would be a step forward. Apart from that, I also think it's good for features to have regression test coverage, and since we use COSTS OFF everywhere or at least nearly everywhere in the regression test, if we don't print out the disabled node counts when COSTS OFF is used, then we don't cover that case in our tests. Bummer. Regression test changes in 0001: - btree_index.sql executes a query "select proname from pg_proc where proname ilike 'ri%foo' order by 1" with everything but bitmap scans disabled. Currently, that produces an index-only scan; with the patch, it produces a sort over a sequential scan. That's a little odd, because the test seems to be aimed at demonstrating that we can use a bitmap scan, and it doesn't, because we apparently can't. But, why does the patch change the plan? At least on my machine, the index-only scan is significantly more costly than the sequential scan. I think what's happening here is that when you add disable_cost to the cost of both paths, they compare fuzzily the same; without that, the cheaper one wins. - select_parallel.out executes a query with sequential scans disabled but tenk2 must nevertheless be sequential-scanned. With the patch, that changes to a parallel sequential scan. I think the explanation here is the same as in the preceding case. - horizons.spec currently sets enable_seqscan=false, enable_indexscan=false, and enable_bitmapscan=false. I suspect that Andres thought that this would force the use of an index-only scan, since nothing sets enable_indexonlyscan=false. But as discussed upthread, that is not true. Instead everything is disabled. For the same reasons as in the previous two examples, this caused an assortment of plan changes which in turn caused the test to fail to test what it was intended to test. So I removed enable_indexscan=false from the spec file, and now it gets index-only scans everywhere again, as desired. -- Robert Haas EDB: http://www.enterprisedb.com
v4-0002-Show-number-of-disabled-nodes-in-EXPLAIN-ANALYZE-.patch
Description: Binary data
v4-0001-Treat-number-of-disabled-nodes-in-a-path-as-a-sep.patch
Description: Binary data
