Re: On disable_cost

On Thu, 4 Apr 2024 at 08:21, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I wanted to further explore the idea of just not generating plans of
> types that are currently disabled. I looked into doing this for
> enable_indexscan and enable_indexonlyscan. As a first step, I
> investigated how those settings work now, and was horrified. I don't
> know whether I just wasn't paying attention back when the original
> index-only scan work was done -- I remember discussing
> enable_indexonlyscan with you at the time -- or whether it got changed
> subsequently. Anyway, the current behavior is:
>
> [A] enable_indexscan=false adds disable_cost to the cost of every
> Index Scan path *and also* every Index-Only Scan path. So disabling
> index-scans also in effect discourages the use of index-only scans,
> which would make sense if we didn't have a separate setting called
> enable_indexonlyscan, but we do. Given that, I think this is
> completely and utterly wrong.
>
> [b] enable_indexonlyscan=false causes index-only scan paths not to be
> generated at all, but instead, we generate index-scan paths to do the
> same thing that we would not have generated otherwise.

FWIW, I think changing this is a bad idea and I don't think the
behaviour that's in your patch is useful. With your patch, if I SET
enable_indexonlyscan=false, any index that *can* support an IOS for my
query will now not be considered at all!

With your patch applied, I see:

-- default enable_* GUC values.
postgres=# explain select oid from pg_class order by oid;
QUERY PLAN
-------------------------------------------------------------------------------------------
Index Only Scan using pg_class_oid_index on pg_class
(cost=0.27..22.50 rows=415 width=4)
(1 row)

postgres=# set enable_indexonlyscan=0; -- no index scan?
SET
postgres=# explain select oid from pg_class order by oid;
QUERY PLAN
-----------------------------------------------------------------
Sort (cost=36.20..37.23 rows=415 width=4)
Sort Key: oid
-> Seq Scan on pg_class (cost=0.00..18.15 rows=415 width=4)
(3 rows)

postgres=# set enable_seqscan=0; -- still no index scan!
SET
postgres=# explain select oid from pg_class order by oid;
QUERY PLAN
------------------------------------------------------------------------------------
Sort (cost=10000000036.20..10000000037.23 rows=415 width=4)
Sort Key: oid
-> Seq Scan on pg_class (cost=10000000000.00..10000000018.15
rows=415 width=4)
(3 rows)

postgres=# explain select oid from pg_class order by oid,relname; --
now an index scan?!
QUERY PLAN
---------------------------------------------------------------------------------------------
Incremental Sort (cost=0.43..79.50 rows=415 width=68)
Sort Key: oid, relname
Presorted Key: oid
-> Index Scan using pg_class_oid_index on pg_class
(cost=0.27..60.82 rows=415 width=68)
(4 rows)

I don't think this is good as pg_class_oid_index effectively won't be
used as long as the particular query could use that index with an
index *only* scan. You can see above that as soon as I adjust the
query slightly so that IOS isn't possible, the index can be used
again. I think an Index Scan would have been a much better option for
the 2nd query than the seq scan and sort.

I think if I do SET enable_indexonlyscan=0; the index should still be
used with an Index Scan and it definitely shouldn't result in Index
Scan also being disabled if that index happens to contain all the
columns required to support an IOS.

FWIW, I'm fine with the current behaviour. It looks like we've
assumed that, when possible, IOS are always superior to Index Scan, so
there's no point in generating an Index Scan path when we can generate
an IOS path. I think this makes sense. For that not to be true,
checking the all visible flag would have to be more costly than
visiting the heap. Perhaps that could be true if the visibility map
page had to come from disk and the heap page was cached and the disk
was slow, but I don't think that scenario is worthy of considering
both Index scan and IOS path types when IOS is possible. We've no way
to accurately cost that anyway.

This all seems similar to enable_sort vs enable_incremental_sort. For
a while, we did consider both an incremental sort and a sort when an
incremental sort was possible, but it seemed to me that an incremental
sort would always be better when it was possible, so I changed that in
4a29eabd1. I've not seen anyone complain. I made it so that when an
incremental sort is possible but is disabled, we do a sort instead.
That seems fairly similar to how master handles
enable_indexonlyscan=false.

In short, I don't find it strange that disabling one node type results
in considering another type that we'd otherwise not consider in cases
where we assume that the disabled node type is always superior and
should always be used when it is possible.

I do agree that adding disable_cost to IOS when enable_indexscan=0 is
a bit weird. We don't penalise incremental sorts when sorts are
disabled, so aligning those might make sense.

David