Subject: Re: Proposal to Enable/Disable Index using ALTER INDEX

From: David Rowley <[email-masked]>
Date: Wed, 11 Sep 2024 09:35:16 +1200

On Wed, 11 Sept 2024 at 03:12, Nathan Bossart <[email-masked]> wrote:
>
> On Tue, Sep 10, 2024 at 10:16:34AM +1200, David Rowley wrote:
> > If we get the skip scan feature for PG18, then there's likely going to
> > be lots of people with indexes that they might want to consider
> > removing after upgrading. Maybe this is a good time to consider this
> > feature as it possibly won't ever be more useful than it will be after
> > we get skip scans.
>
> +1, this is something I've wanted for some time.  There was some past
> discussion, too [0].
>
> [0] https://postgr.es/m/flat/ed8c9ed7-bb5d-aaec-065b-ad4893645deb%402ndQuadrant.com

Thanks for digging that up. I'd forgotten about that.  I see there was
pushback from having this last time, which is now over 6 years ago.
In the meantime, we still have nothing to make this easy for people.

I think the most important point I read in that thread is [1]. Maybe
what I mentioned in [2] is a good workaround.

Additionally, I think there will need to be syntax in CREATE INDEX for
this. Without that pg_get_indexdef() might return SQL that does not
reflect the current state of the index. MySQL seems to use "CREATE
INDEX name ON table (col) [VISIBLE|INVISIBLE]".

David

[1] https://www.postgresql.org/message-id/20180618215635.m5vrnxdxhxytvmcm%40alap3.anarazel.de
[2] https://www.postgresql.org/message-id/CAKJS1f_L7y_BTGESp5Qd6BSRHXP0mj3x9O9C_U27GU478UwpBw%40mail.gmail.com

---

From: Shayon Mukherjee <[email-masked]>
Date: Sun, 22 Sep 2024 13:42:48 -0400

Hello,

Thank you for all the feedback and insights. Work was busy, so I didn't get
to follow up earlier.

This patch introduces the ability to enable or disable indexes using ALTER
INDEX
and CREATE INDEX commands.

Original motivation for the problem and proposal for a patch
can be found here[0]

This patch contains the relevant implementation details, new regression
tests and documentation.
It passes all the existing specs and the newly added regression tests. It
compiles, so the
patch can be applied for testing as well.

I have attached the patch in this email, and have also shared it on my
Github fork[1]. Mostly so
that I can ensure the full CI passes.

*Implementation details:*
- New Grammar:
  * ALTER INDEX ... ENABLE/DISABLE
  * CREATE INDEX ... DISABLE

- Default state is enabled. Indexes are only disabled when explicitly
  instructed via CREATE INDEX ... DISABLE or ALTER INDEX ... DISABLE.

- Primary Key and Unique constraint indexes are always enabled as well. The
  ENABLE/DISABLE grammar is not supported for these types of indexes. They
can
  be later disabled via ALTER INDEX ... ENABLE/DISABLE.

- ALTER INDEX ... ENABLE/DISABLE performs an in-place update of the
pg_index
  catalog to protect against indcheckxmin.

- pg_get_indexdef() support for the new functionality and grammar. This
change is
  reflected in \d output for tables and pg_dump. We show the DISABLE syntax
accordingly.

- Updated create_index.sql regression test to cover the new grammar and
verify
  that disabled indexes are not used in queries.

- Modified get_index_paths() and build_index_paths() to exclude disabled
  indexes from consideration during query planning.

- No changes are made to stop the index from getting rebuilt. This way we
ensure no
  data miss or corruption when index is re-enabled.

- TOAST indexes are supported and enabled by default.

- REINDEX CONCURRENTLY is supported as well and the existing state of
pg_index.indisenabled
  is carried over accordingly.

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

- See the changes in create_index.sql to get an idea of the grammar and sql
statements.

- See the changes in create_index.out to get an idea of the catalogue
states and EXPLAIN
  output to see when an index is getting used or isn't (when disabled).

I am looking forward to any and all feedback on this patch, including but
not limited to
code quality, tests, and fundamental logic.

Thank you for the reviews and feedback.

[0]
https://www.postgresql.org/message-id/CANqtF-oXKe0M%3D0QOih6H%2BsZRjE2BWAbkW_1%2B9nMEAMLxUJg5jA%40mail.gmail.com
[1] https://github.com/shayonj/postgres/pull/1

Best,
Shayon

On Tue, Sep 10, 2024 at 5:35 PM David Rowley <[email-masked]> wrote:

> > [0]
> https://postgr.es/m/flat/ed8c9ed7-bb5d-aaec-065b-ad4893645deb%402ndQuadrant.com
>
> [1]
> https://www.postgresql.org/message-id/20180618215635.m5vrnxdxhxytvmcm%40alap3.anarazel.de
> [2]
> https://www.postgresql.org/message-id/CAKJS1f_L7y_BTGESp5Qd6BSRHXP0mj3x9O9C_U27GU478UwpBw%40mail.gmail.com
>

-- 
Kind Regards,
Shayon Mukherjee

---

From: Shayon Mukherjee <[email-masked]>
Date: Sun, 22 Sep 2024 14:20:53 -0400

Hello,

I realized there were some white spaces in the diff and a compiler warning
error from CI, so I have fixed those and the updated patch with v2 is now
attached.

Shayon

On Sun, Sep 22, 2024 at 1:42 PM Shayon Mukherjee <[email-masked]> wrote:

> Hello,
>
> Thank you for all the feedback and insights. Work was busy, so I didn't
> get to follow up earlier.
>
>   reflected in \d output for tables and pg_dump. We show the DISABLE
> syntax accordingly.
>
> - See the changes in create_index.sql to get an idea of the grammar and
> sql statements.
>

-- 
Kind Regards,
Shayon Mukherjee

---

From: David Rowley <[email-masked]>
Date: Mon, 23 Sep 2024 10:44:58 +1200

>   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

---

From: Shayon Mukherjee <[email-masked]>
Date: Mon, 23 Sep 2024 07:07:11 -0400

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 <[email-masked]> wrote:
>