Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX
On Thu, Apr 24, 2025 at 12:45 AM jian he
wrote:
> hi.
> The following is a review of version 17.
>
> ATExecSetIndexVisibility
> if (indexForm->indisvisible != visible)
> {
> indexForm->indisvisible = visible;
> CatalogTupleUpdate(pg_index, &indexTuple->t_self, indexTuple);
> CacheInvalidateRelcache(heapRel);
> InvokeObjectPostAlterHook(IndexRelationId, indexOid, 0);
> CommandCounterIncrement();
> }
> I am slightly confused. if we already used
> CommandCounterIncrement();
> then we don't need CacheInvalidateRelcache?
>
>
>
Thank you for this catch. I misunderstood the behavior of the two and was
performing both to avoid inconsistency between state within a transaction
and cross session, but as you pointed out CommandCounterIncrement() helps
achieve both. Updated.
doc/src/sgml/ref/alter_index.sgml
>
>ALTER INDEX changes the definition of an existing
> index.
>There are several subforms described below. Note that the lock level
> required
>may differ for each subform. An ACCESS EXCLUSIVE
> lock is held
>unless explicitly noted. When multiple subcommands are listed, the lock
>held will be the strictest one required from any subcommand.
>
> per the above para, we need mention that ALTER INDEX SET
> INVISIBLE|INVISIBLE
> only use ShareUpdateExclusiveLock?
>
>
I wasn't sure at first where to add the note about
ShareUpdateExclusiveLock. But it looks like we have a precedent from RENAME
in doc/src/sgml/ref/alter_index.sgml, so I have done the same for VISIBLE &
INVISIBLE in doc/src/sgml/ref/alter_index.sgml as well.
> index_create is called in several places,
> most of the time, we use INDEX_CREATE_VISIBLE.
> if we define it as INDEX_CREATE_INVISIBLE rather than INDEX_CREATE_VISIBLE
> then argument flags required code changes would be less, (i didn't try
> it myself)
>
Looks like the only change we would save is the one in
src/backend/catalog/toasting.c. Rest of the code change/diffs would still
be needed IIUC (if I understand correctly). This approach felt a bit
ergonomical, hence opted for it, but happy to update. Let me know.
>
Similar to get_index_isclustered,
> We can place GetIndexVisibility in
> src/backend/utils/cache/lsyscache.c,
> make it an extern function, so others can use it;
> to align with other function names,
> maybe rename it as get_index_visibility.
>
>
I was a bit torn on this one and figured I wouldn't introduce it as it
could be a bit of premature optimization, until there were more use cases
(or maybe one more). Plus, I figured the next time we need this info, we
could expose a more public function like get_index_visibility (given N=2, N
being the number of callers). However, given you mentioned and spotted this
as well, I have introduced get_index_visibility in the new patch now.
> create index v2_idx on v1(data) visible;
> is allowed,
> doc/src/sgml/ref/create_index.sgml
> section need to change to
> [ VISIBLE | INVISIBLE ]
>
> ?
>
Updated to match the same pattern as the one in
doc/src/sgml/ref/alter_index.sgml.
Thank you for the feedback. I have also updated the feedback from [1] as
well. Few extra notes:
- Attached v18
- Rebased against master
- Updated the commit message
- Updated the target remote version to now be fout->remoteVersion >= 19
- Using a UNION ALL query to show all indexes from part_tbl partitioned
tables in the specs as noted in [1]. The query suggested in [1] wasn't
encompassing all the indexes, hence the UNION ALL for WHERE i.indrelid =
'part_tbl'::regclass::oid.
[1]
https://www.postgresql.org/message-id/cacjufxfs_m7ngvfiz-duutawb7rqxrmo97wc5zezkw2zsmq...@mail.gmail.com
Thank you
Shayon
From ee130c6ec2555ecdc2cb8e3eda360017507c1e4f Mon Sep 17 00:00:00 2001
From: Shayon Mukherjee
Date: Sun, 27 Apr 2025 16:37:41 -0400
Subject: [PATCH v18] Introduce the ability to set index visibility using ALTER
INDEX
This patch introduces the ability to make an index INVISIBLE ( or VISIBLE )
to the planner, making the index not eligible for planning but will continue
to be maintained when the underlying data changes. This behavior is accomplished
by introducing new grammar ALTER INDEX ... VISIBLE|INVISIBLE and
CREATE INDEX ... VISIBLE|INVISIBLE.
Additionally, there is also support for force_invisible_index GUC parameter that forces
the planner to use invisible indexes. This is useful for testing and validating index behavior
without changing their visibility state.
Discussion: https://www.postgresql.org/message-id/flat/EF2313B8-A017-4869-9B7F-A24EDD8795DE%40gmail.com#dbe65017ffa7b65a4f3f29e64ed2fce5
---
doc/src/sgml/catalogs.sgml| 11 +
doc/src/sgml/config.sgml | 16 +
doc/src/sgml/indices.sgml | 21 +
doc/src/sgml/ref/alter_index.sgml | 40 +
doc/src/sgml/ref/create_index.sgml| 29 +
src/backend/bootstrap/bootparse.y | 2 +
src/backend/catalog/index.c | 31 +-
src
Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX
hi, two more minor issues. src/bin/pg_dump/pg_dump.c if (fout->remoteVersion >= 18) need change to if (fout->remoteVersion >= 19) +-- Test index visibility with partitioned tables +CREATE TABLE part_tbl(id int, data text) PARTITION BY RANGE(id); +CREATE TABLE part1 PARTITION OF part_tbl FOR VALUES FROM (1) TO (100); +CREATE TABLE part2 PARTITION OF part_tbl FOR VALUES FROM (100) TO (200); +INSERT INTO part_tbl +SELECT g, 'data ' || g +FROM generate_series(1, 199) g; +CREATE INDEX idx_part_tbl ON part_tbl(data); +SELECT c.relname, i.indisvisible +FROM pg_index i +JOIN pg_class c ON i.indexrelid = c.oid +WHERE c.relname LIKE 'idx_part_tbl%' +ORDER BY c.relname; + relname| indisvisible +--+-- + idx_part_tbl | t +(1 row) + This test seems not that good? "idx_part_tbl" is the partitioned index, we also need to show each partition index pg_index.indisvisible value? we can change it to CREATE TABLE part_tbl(id int, data text) PARTITION BY RANGE(id); CREATE TABLE part_tbl_1 PARTITION OF part_tbl FOR VALUES FROM (1) TO (100); CREATE TABLE part_tbl_2 PARTITION OF part_tbl FOR VALUES FROM (100) TO (200); CREATE INDEX ON part_tbl(data); SELECT c.relname, i.indisvisible FROM pg_index i JOIN pg_class c ON i.indexrelid = c.oid WHERE c.relname LIKE 'part_tbl%' ORDER BY c.relname; -
Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX
hi.
The following is a review of version 17.
ATExecSetIndexVisibility
if (indexForm->indisvisible != visible)
{
indexForm->indisvisible = visible;
CatalogTupleUpdate(pg_index, &indexTuple->t_self, indexTuple);
CacheInvalidateRelcache(heapRel);
InvokeObjectPostAlterHook(IndexRelationId, indexOid, 0);
CommandCounterIncrement();
}
I am slightly confused. if we already used
CommandCounterIncrement();
then we don't need CacheInvalidateRelcache?
doc/src/sgml/ref/alter_index.sgml
ALTER INDEX changes the definition of an existing index.
There are several subforms described below. Note that the lock level required
may differ for each subform. An ACCESS EXCLUSIVE
lock is held
unless explicitly noted. When multiple subcommands are listed, the lock
held will be the strictest one required from any subcommand.
per the above para, we need mention that ALTER INDEX SET INVISIBLE|INVISIBLE
only use ShareUpdateExclusiveLock?
index_create is called in several places,
most of the time, we use INDEX_CREATE_VISIBLE.
if we define it as INDEX_CREATE_INVISIBLE rather than INDEX_CREATE_VISIBLE
then argument flags required code changes would be less, (i didn't try
it myself)
Similar to get_index_isclustered,
We can place GetIndexVisibility in
src/backend/utils/cache/lsyscache.c,
make it an extern function, so others can use it;
to align with other function names,
maybe rename it as get_index_visibility.
create index v2_idx on v1(data) visible;
is allowed,
doc/src/sgml/ref/create_index.sgml
section need to change to
[ VISIBLE | INVISIBLE ]
?
Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX
On Mon, Apr 7, 2025 at 5:39 PM Sami Imseih wrote:
> > Attached v16 with feedback and rebased.
>
> Thanks, and I realized the original tab-complete I proposed
> was not entirely correct. I fixed it and also shortened the
> commit message.
I was wondering about if the check needed to be more encompassing. Your
proposal definitely makes sense, thank you!
+ else if (TailMatches("INDEX|CONCURRENTLY", "ON", MatchAny, "USING",
MatchAny, "(*)")
Shayon
Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX
> Attached v16 with feedback and rebased. Thanks, and I realized the original tab-complete I proposed was not entirely correct. I fixed it and also shortened the commit message. -- Sami Imseih Amazon Web Services (AWS) v17-0001-Introduce-the-ability-to-set-index-visibility-us.patch Description: Binary data
Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX
On Mon, Apr 7, 2025 at 4:01 PM Sami Imseih wrote:
> Thanks for the update!
>
> The changes in v15 look good to me. The patch does need to be rebased,
> and I also think you should add a tab-complete for CREATE INDEX
>
>
> simseih@bcd07415af92 postgresql % git diff
> diff --git a/src/bin/psql/tab-complete.in.c
> b/src/bin/psql/tab-complete.in.c
> index 8e2eb50205e..f1853a68ccc 100644
> --- a/src/bin/psql/tab-complete.in.c
> +++ b/src/bin/psql/tab-complete.in.c
> @@ -3434,6 +3434,8 @@ match_previous_words(int pattern_id,
> !TailMatches("POLICY", MatchAny, MatchAny,
> MatchAny, MatchAny, MatchAny) &&
> !TailMatches("FOR", MatchAny, MatchAny, MatchAny))
> COMPLETE_WITH("(");
> + else if (TailMatches("*)"))
> + COMPLETE_WITH("VISIBLE", "INVISIBLE");
>
> /* CREATE OR REPLACE */
> else if (Matches("CREATE", "OR"))
>
> IMO, with the above in place, this patch is RFC.
>
>
>
Thank you Sami, really appreciate it!
Attached v16 with feedback and rebased.
Thanks
Shayon
From b1833753476d2c7d062c6d3900b2671a09d32e12 Mon Sep 17 00:00:00 2001
From: Shayon Mukherjee
Date: Sun, 12 Jan 2025 14:34:48 -0500
Subject: [PATCH v16] Introduce the ability to set index visibility using ALTER
INDEX
This patch introduces index visibility control using ALTER INDEX and CREATE INDEX
commands.
Original motivation for the problem and proposal for a patch
can be found at [1].
This patch passes all the existing specs and the newly added regression tests. The patch
is ready for review and test. It compiles, so the patch can be applied for testing as well.
Note: The patch has gone through a few iterations. Earlier versions of the patch had the ENABLE/DISABLE grammar.
The current version has the VISIBLE/INVISIBLE grammar. So, you will the local variable names using the new grammar accordingly.
Implementation details:
- New Grammar:
* ALTER INDEX ... VISIBLE/INVISIBLE
* CREATE INDEX ... INVISIBLE
- Default state is visible. Indexes are only invisible when explicitly
instructed via CREATE INDEX ... INVISIBLE or ALTER INDEX ... INVISIBLE.
- Primary Key and Unique constraint indexes are always visible. The
VISIBLE/INVISIBLE grammar is supported for these types of indexes and they can
be made invisible via ALTER INDEX ... INVISIBLE.
- ALTER INDEX ... VISIBLE/INVISIBLE performs update on the relevant row in pg_index
catalog
- pg_get_indexdef() supports the new functionality and grammar. This change is
reflected in \d output for tables and pg_dump. We show the INVISIBLE syntax accordingly.
- Added force_invisible_index GUC parameter that forces the planner to use invisible
indexes. This is useful for testing and validating index behavior without changing
their visibility state. Based on feedback from Sami S [2]
- Updated create_index.sql regression test to cover the new grammar and verify
that invisible indexes are not used in queries. The test covers:
- Basic single-column and multi-column indexes
- Partial indexes
- Expression indexes
- Join indexes
- GIN and GiST indexes
- Covering indexes
- Range indexes
- Unique indexes and constraints
- Adds a new indisvisible attribute to the IndexOptInfo structure.
- Modifies get_relation_info in plancat.c to skip invisible indexes entirely, thus reducing the number of places we need to check if an index is invisible or not. Inspired by the conversations start at [3].
- I chose to modify the logic within get_relation_info as compared to, say, reducing the cost to make the planner not consider an index during planning, mostly to keep the number of changes being introduced to a minimum and also the logic itself being self-contained and easier to understand perhaps (?).
- No changes are made to stop the index from getting maintained. This way we ensure no
data loss or corruption when index is made visible again.
- TOAST indexes are supported and visible by default as well.
- REINDEX CONCURRENTLY is supported as well and existing state of pg_index.indisvisible
is carried over accordingly.
- 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 invisible).
- Incorporated DavidR's feedback from [4] around documentation and also you will see that by skipping invisible indexes entirely from get_relation_info in plancat.c (as mentioned above), we address the other mentioned issues as well.
- Lastly, protects against the case where indcheckxmin is true by raising ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE.
Looking forward to any and all feedback on this patch, including but not limited to code quality, tests, fundamental logic.
[1] https://www.postgresql.org/message-id/CANqtF-oXKe0M%3D0QOih6H%2BsZRjE2BWAbkW_1%2B9nMEAMLxUJg5jA%40mail.gmail.com
[2] https://www.postgresql.org/me
Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX
Thanks for the update!
The changes in v15 look good to me. The patch does need to be rebased,
and I also think you should add a tab-complete for CREATE INDEX
simseih@bcd07415af92 postgresql % git diff
diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index 8e2eb50205e..f1853a68ccc 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -3434,6 +3434,8 @@ match_previous_words(int pattern_id,
!TailMatches("POLICY", MatchAny, MatchAny,
MatchAny, MatchAny, MatchAny) &&
!TailMatches("FOR", MatchAny, MatchAny, MatchAny))
COMPLETE_WITH("(");
+ else if (TailMatches("*)"))
+ COMPLETE_WITH("VISIBLE", "INVISIBLE");
/* CREATE OR REPLACE */
else if (Matches("CREATE", "OR"))
IMO, with the above in place, this patch is RFC.
--
Sami Imseih
Amazon Web Services (AWS)
Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX
On Wed, Apr 2, 2025 at 10:53 PM Sami Imseih wrote: > > That seems like a very good location for this advice. But the current > > set of bullet points are all directed towards "... a general procedure > > for determining which indexes to create". I propose that a new paragrph, > > not a bullet point, be added towards the end of that section which > > addresses the options of steps to take before dropping an index. > > Something like the following: > > > Thoughts? > > This new feature provides the ability to experiment with indexes to > create ( or drop ), > so I don't think it deviates from the purpose of this paragraphs. > > This patch will provide the ability for the user to create an index as > initially > invisible and a GUC, use_invisible_index if set to TRUE to experiment with > the new index to see if it improves performance. So, I think providing this > pattern to experiment with a new index will fit nicely as a new > bulletpoint. > > > Thank you for the feedback and pointers Sami and Gurjeet. Good call on [0] being a good place for operational advice. I have gone ahead and removed the advice about "pg_stat_user_indexes.idx_scan" from doc/src/sgml/ref/alter_index.sgml and updated doc/src/sgml/indices.sgml to include a new bullet point with also a reference to use_invisible_index. Let me know how it sounds and if there is any feedback. Also, rebased. Thank you Shayon [0] https://www.postgresql.org/docs/current/indexes-examine.html v15-0001-Introduce-the-ability-to-set-index-visibility-us.patch Description: Binary data
Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX
On Wed Apr 2, 2025 at 6:58 PM PDT, Sami Imseih wrote: >> > + indexes. If performance degrades after making an index invisible, it can >> > be easily >> > + be made visible using VISIBLE. Before making an index >> > invisible, it's recommended >> > + to check >> > pg_stat_user_indexes.idx_scan >> > + to identify potentially unused indexes. >> >> I feel ALTER INDEX command reference doc is not the right place for this >> kind of >> operational advice. Is there a better place in documentation for this kind of >> advice? Or maybe it needs to be reworded to fit the command reference style? > > I agree with you. > > What about we add this wording in the following section [0]? This > section discusses techniques > for experimenting with indexes. It says, > " A good deal of experimentation is often necessary. The rest of > this section gives some tips for that:" > > A discussion about invisible indexes as one of the tools for > experimentation can be added here. > What do you think? > > [0] https://www.postgresql.org/docs/current/indexes-examine.html That seems like a very good location for this advice. But the current set of bullet points are all directed towards "... a general procedure for determining which indexes to create". I propose that a new paragrph, not a bullet point, be added towards the end of that section which addresses the options of steps to take before dropping an index. Something like the following: Sometimes you may notice that an index is not being used anymore by the application queries. In such cases, it is a good idea to investigate if such an index can be dropped, because an index that is not being used for query optimization still consumes resources and slows down INSERT, UPDATE, and DELETE commands. To aid in such an investigation, look at the pg_stat_user_indexes.idx_scan count for the index. To determine the performance effects of dropping the index, without actually dropping the said index, you may mark the index invisible to the planner by using the ALTER INDEX ... INVISIIBLE command. If it turns out that doing so causes a performance degradation, the index can be quickly made visible to the planner for query optimization by using the ALTER INDEX ... VISIBLE command. Thoughts? Best regards, Gurjeet http://Gurje.et
Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX
> That seems like a very good location for this advice. But the current > set of bullet points are all directed towards "... a general procedure > for determining which indexes to create". I propose that a new paragrph, > not a bullet point, be added towards the end of that section which > addresses the options of steps to take before dropping an index. > Something like the following: > Thoughts? This new feature provides the ability to experiment with indexes to create ( or drop ), so I don't think it deviates from the purpose of this paragraphs. This patch will provide the ability for the user to create an index as initially invisible and a GUC, use_invisible_index if set to TRUE to experiment with the new index to see if it improves performance. So, I think providing this pattern to experiment with a new index will fit nicely as a new bulletpoint. -- Sami Imseih Amazon Web Services (AWS)
Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX
> > + indexes. If performance degrades after making an index invisible, it can > > be easily > > + be made visible using VISIBLE. Before making an index > > invisible, it's recommended > > + to check > > pg_stat_user_indexes.idx_scan > > + to identify potentially unused indexes. > > I feel ALTER INDEX command reference doc is not the right place for this kind > of > operational advice. Is there a better place in documentation for this kind of > advice? Or maybe it needs to be reworded to fit the command reference style? I agree with you. What about we add this wording in the following section [0]? This section discusses techniques for experimenting with indexes. It says, " A good deal of experimentation is often necessary. The rest of this section gives some tips for that:" A discussion about invisible indexes as one of the tools for experimentation can be added here. What do you think? [0] https://www.postgresql.org/docs/current/indexes-examine.html -- Sami Imseih Amazon Web Services (AWS)
Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX
On Sun, Sep 22, 2024 at 3:45 PM David Rowley wrote: > 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) While reviewing Shayon's v14 patch, I had removed text (quoted below) from the ALTER INDEX docs that did not feel right in a command reference. I thought of reading up on the history/discussion of the patch, and now I see why Shayon chose to include an advice in ALTER INDEX docs. > + indexes. If performance degrades after making an index invisible, it can be > easily > + be made visible using VISIBLE. Before making an index > invisible, it's recommended > + to check > pg_stat_user_indexes.idx_scan > + to identify potentially unused indexes. I feel ALTER INDEX command reference doc is not the right place for this kind of operational advice. Is there a better place in documentation for this kind of advice? Or maybe it needs to be reworded to fit the command reference style? Best regards, Gurjeet http://Gurje.et
Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX
On Mon, 23 Sept 2024 at 05:43, Shayon Mukherjee 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
Re: [PATCH] Re: Proposal to Enable/Disable Index using ALTER INDEX
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 wrote: > 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 wrote: > >> On Wed, 11 Sept 2024 at 03:12, Nathan Bossart >> 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 >> > > > -- > Kind Regards, > Shayon Mukherjee > -- Kind Regards, Shayon Mukherjee v2-0001-Introduce-the-ability-to-enable-disable-indexes.patch Description: Binary data
