Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX

On Mon, 23 Sept 2024 at 05:43, Shayon Mukherjee <shayonj(at)gmail(dot)com> wrote:
> - Modified get_index_paths() and build_index_paths() to exclude disabled
> indexes from consideration during query planning.

There are quite a large number of other places you also need to modify.

Here are 2 places where the index should be ignored but isn't:

1. expression indexes seem to still be used for statistical estimations:

create table b as select generate_series(1,1000)b;
create index on b((b%10));
analyze b;
explain select distinct b%10 from b;
-- HashAggregate (cost=23.00..23.12 rows=10 width=4)

alter index b_expr_idx disable;
explain select distinct b%10 from b;
-- HashAggregate (cost=23.00..23.12 rows=10 width=4) <-- should be 1000 rows

drop index b_expr_idx;
explain select distinct b%10 from b;
-- HashAggregate (cost=23.00..35.50 rows=1000 width=4)

2. Indexes seem to still be used for join removals.

create table c (c int primary key);
explain select c1.* from c c1 left join c c2 on c1.c=c2.c; --
correctly removes join.
alter index c_pkey disable;
explain select c1.* from c c1 left join c c2 on c1.c=c2.c; -- should
not remove join.

Please carefully look over all places that RelOptInfo.indexlist is
looked at and consider skipping disabled indexes. Please also take
time to find SQL that exercises each of those places so you can verify
that the behaviour is correct after your change. This is also a good
way to learn exactly all cases where indexes are used. Using this
method would have led you to find places like
rel_supports_distinctness(), where you should be skipping disabled
indexes.

The planner should not be making use of disabled indexes for any
optimisations at all.

> - catversion.h is updated with a new CATALOG_VERSION_NO to reflect change in pg_index
> schema.

Please leave that up to the committer. Patch authors doing this just
results in the patch no longer applying as soon as someone commits a
version bump.

Also, please get rid of these notices. The command tag serves that
purpose. It's not interesting that the index is already disabled.

# alter index a_pkey disable;
NOTICE: index "a_pkey" is now disabled
ALTER INDEX
# alter index a_pkey disable;
NOTICE: index "a_pkey" is already disabled
ALTER INDEX

I've only given the code a very quick glance. I don't quite understand
why you're checking the index is enabled in create_index_paths() and
get_index_paths(). I think the check should be done only in
create_index_paths(). Primarily, you'll find code such as "if
(index->indpred != NIL && !index->predOK)" in the locations you need
to consider skipping the disabled index. I think your new code should
be located very close to those places or perhaps within the same if
condition unless it makes it overly complex for the human reader.

I think the documents should also mention that disabling an index is a
useful way to verify an index is not being used before dropping it as
the index can be enabled again at the first sign that performance has
been effected. (It might also be good to mention that checking
pg_stat_user_indexes.idx_scan should be the first port of call when
checking for unused indexes)

David