Re: On disable_cost

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