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

Hi David,

Thank you so much for the review and pointers. I totally missed expression indexes. I am going to do another proper pass along with your feedback and follow up with an updated patch and any questions.

Excited to be learning so much about the internals.
Shayon

> On Sep 22, 2024, at 6:44 PM, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> 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