public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v7 2/7] Implement CLUSTER of partitioned table..
5+ messages / 5 participants
[nested] [flat]
* [PATCH v7 2/7] Implement CLUSTER of partitioned table..
@ 2020-06-07 21:58 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Justin Pryzby @ 2020-06-07 21:58 UTC (permalink / raw)
This requires either specification of a partitioned index on which to cluster,
or that an partitioned index was previously set clustered.
---
doc/src/sgml/ref/cluster.sgml | 6 +
src/backend/commands/cluster.c | 174 +++++++++++++++++++-------
src/bin/psql/tab-complete.c | 1 +
src/include/commands/cluster.h | 1 +
src/test/regress/expected/cluster.out | 58 ++++++++-
src/test/regress/sql/cluster.sql | 24 +++-
6 files changed, 209 insertions(+), 55 deletions(-)
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 5dd21a0189..fb5deddb35 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -192,6 +192,12 @@ CLUSTER [VERBOSE]
are periodically reclustered.
</para>
+ <para>
+ Clustering a partitioned table clusters each of its partitions using the
+ index partition of the given partitioned index or (if not specified) the
+ partitioned index marked as clustered.
+ </para>
+
</refsect1>
<refsect1>
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 096a06f7b3..9b6673867c 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -32,7 +32,9 @@
#include "catalog/index.h"
#include "catalog/namespace.h"
#include "catalog/objectaccess.h"
+#include "catalog/partition.h"
#include "catalog/pg_am.h"
+#include "catalog/pg_inherits.h"
#include "catalog/toasting.h"
#include "commands/cluster.h"
#include "commands/defrem.h"
@@ -73,6 +75,9 @@ static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
bool verbose, bool *pSwapToastByContent,
TransactionId *pFreezeXid, MultiXactId *pCutoffMulti);
static List *get_tables_to_cluster(MemoryContext cluster_context);
+static List *get_tables_to_cluster_partitioned(MemoryContext cluster_context,
+ Oid indexOid);
+static void cluster_multiple_rels(List *rvs, int options);
/*---------------------------------------------------------------------------
@@ -135,7 +140,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
AccessExclusiveLock,
0,
RangeVarCallbackOwnsTable, NULL);
- rel = table_open(tableOid, NoLock);
+ rel = table_open(tableOid, ShareUpdateExclusiveLock);
/*
* Reject clustering a remote temp table ... their local buffer
@@ -146,14 +151,6 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot cluster temporary tables of other sessions")));
- /*
- * Reject clustering a partitioned table.
- */
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot cluster a partitioned table")));
-
if (stmt->indexname == NULL)
{
ListCell *index;
@@ -189,10 +186,34 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
}
/* close relation, keep lock till commit */
- table_close(rel, NoLock);
+ table_close(rel, ShareUpdateExclusiveLock);
- /* Do the job. */
- cluster_rel(tableOid, indexOid, ¶ms);
+ if (rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
+ {
+ /* Do the job. */
+ cluster_rel(tableOid, indexOid, ¶ms);
+ }
+ else
+ {
+ List *rvs;
+ MemoryContext cluster_context;
+
+ /* Refuse to hold strong locks in a user transaction */
+ PreventInTransactionBlock(isTopLevel, "CLUSTER");
+
+ cluster_context = AllocSetContextCreate(PortalContext,
+ "Cluster",
+ ALLOCSET_DEFAULT_SIZES);
+
+ rvs = get_tables_to_cluster_partitioned(cluster_context, indexOid);
+ cluster_multiple_rels(rvs, params.options);
+
+ /* Start a new transaction for the cleanup work. */
+ StartTransactionCommand();
+
+ /* Clean up working storage */
+ MemoryContextDelete(cluster_context);
+ }
}
else
{
@@ -202,7 +223,6 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
*/
MemoryContext cluster_context;
List *rvs;
- ListCell *rv;
/*
* We cannot run this form of CLUSTER inside a user transaction block;
@@ -225,28 +245,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
* cluster_context.
*/
rvs = get_tables_to_cluster(cluster_context);
-
- /* Commit to get out of starting transaction */
- PopActiveSnapshot();
- CommitTransactionCommand();
-
- /* Ok, now that we've got them all, cluster them one by one */
- foreach(rv, rvs)
- {
- RelToCluster *rvtc = (RelToCluster *) lfirst(rv);
- ClusterParams cluster_params = params;
-
- /* Start a new transaction for each relation. */
- StartTransactionCommand();
- /* functions in indexes may want a snapshot set */
- PushActiveSnapshot(GetTransactionSnapshot());
- /* Do the job. */
- cluster_params.options |= CLUOPT_RECHECK;
- cluster_rel(rvtc->tableOid, rvtc->indexOid,
- &cluster_params);
- PopActiveSnapshot();
- CommitTransactionCommand();
- }
+ cluster_multiple_rels(rvs, params.options | CLUOPT_RECHECK_ISCLUSTERED);
/* Start a new transaction for the cleanup work. */
StartTransactionCommand();
@@ -352,9 +351,10 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
}
/*
- * Check that the index is still the one with indisclustered set.
+ * Check that the index is still the one with indisclustered set, if needed.
*/
- if (!get_index_isclustered(indexOid))
+ if ((params->options & CLUOPT_RECHECK_ISCLUSTERED) != 0 &&
+ !get_index_isclustered(indexOid))
{
relation_close(OldHeap, AccessExclusiveLock);
pgstat_progress_end_command();
@@ -398,8 +398,13 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
/* Check heap and index are valid to cluster on */
if (OidIsValid(indexOid))
+ {
check_index_is_clusterable(OldHeap, indexOid, recheck, AccessExclusiveLock);
+ /* Mark the index as clustered */
+ mark_index_clustered(OldHeap, indexOid, true);
+ }
+
/*
* Quietly ignore the request if this is a materialized view which has not
* been populated from its query. No harm is done because there is no data
@@ -415,6 +420,14 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
return;
}
+ /* For a partitioned rel, we're done. */
+ if (!RELKIND_HAS_STORAGE(get_rel_relkind(tableOid)))
+ {
+ relation_close(OldHeap, AccessExclusiveLock);
+ pgstat_progress_end_command();
+ return;
+ }
+
/*
* All predicate locks on the tuples or pages are about to be made
* invalid, because we move tuples around. Promote them to relation
@@ -483,6 +496,9 @@ check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMOD
* the worst consequence of following broken HOT chains would be that we
* might put recently-dead tuples out-of-order in the new table, and there
* is little harm in that.)
+ *
+ * This also refuses to cluster on an "incomplete" partitioned index
+ * created with "ON ONLY".
*/
if (!OldIndex->rd_index->indisvalid)
ereport(ERROR,
@@ -507,12 +523,6 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
Relation pg_index;
ListCell *index;
- /* Disallow applying to a partitioned table */
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot mark index clustered in partitioned table")));
-
/*
* If the index is already marked clustered, no need to do anything.
*/
@@ -584,10 +594,6 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
TransactionId frozenXid;
MultiXactId cutoffMulti;
- /* Mark the correct index as clustered */
- if (OidIsValid(indexOid))
- mark_index_clustered(OldHeap, indexOid, true);
-
/* Remember info about rel before closing OldHeap */
relpersistence = OldHeap->rd_rel->relpersistence;
is_system_catalog = IsSystemRelation(OldHeap);
@@ -1582,3 +1588,77 @@ get_tables_to_cluster(MemoryContext cluster_context)
return rvs;
}
+
+/*
+ * Return a List of tables and associated index, where each index is a
+ * partition of the given index
+ */
+static List *
+get_tables_to_cluster_partitioned(MemoryContext cluster_context, Oid indexOid)
+{
+ List *inhoids;
+ ListCell *lc;
+ List *rvs = NIL;
+ MemoryContext old_context;
+
+ inhoids = find_all_inheritors(indexOid, NoLock, NULL);
+
+ foreach(lc, inhoids)
+ {
+ Oid indexrelid = lfirst_oid(lc);
+ Oid relid = IndexGetRelation(indexrelid, false);
+ RelToCluster *rvtc;
+
+ /*
+ * Partitioned rels are also processed by cluster_rel, to
+ * call check_index_is_clusterable() and mark_index_clustered().
+ */
+
+ /*
+ * We have to build the list in a different memory context so it will
+ * survive the cross-transaction processing
+ */
+ old_context = MemoryContextSwitchTo(cluster_context);
+
+ rvtc = (RelToCluster *) palloc(sizeof(RelToCluster));
+ rvtc->tableOid = relid;
+ rvtc->indexOid = indexrelid;
+ rvs = lappend(rvs, rvtc);
+
+ MemoryContextSwitchTo(old_context);
+ }
+
+ return rvs;
+}
+
+/* Cluster each relation in a separate transaction */
+static void
+cluster_multiple_rels(List *rvs, int options)
+{
+ ListCell *lc;
+
+ /* Commit to get out of starting transaction */
+ PopActiveSnapshot();
+ CommitTransactionCommand();
+
+ /* Ok, now that we've got them all, cluster them one by one */
+ foreach(lc, rvs)
+ {
+ RelToCluster *rvtc = (RelToCluster *) lfirst(lc);
+ ClusterParams cluster_params = { .options = options, };
+
+ /* Start a new transaction for each relation. */
+ StartTransactionCommand();
+
+ /* functions in indexes may want a snapshot set */
+ PushActiveSnapshot(GetTransactionSnapshot());
+
+ /* Do the job. */
+ cluster_params.options |= CLUOPT_RECHECK;
+ cluster_rel(rvtc->tableOid, rvtc->indexOid,
+ &cluster_params);
+
+ PopActiveSnapshot();
+ CommitTransactionCommand();
+ }
+}
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index a75647b1cc..781aa95abc 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -588,6 +588,7 @@ static const SchemaQuery Query_for_list_of_clusterables = {
.catname = "pg_catalog.pg_class c",
.selcondition =
"c.relkind IN (" CppAsString2(RELKIND_RELATION) ", "
+ CppAsString2(RELKIND_PARTITIONED_TABLE) ", "
CppAsString2(RELKIND_MATVIEW) ")",
.viscondition = "pg_catalog.pg_table_is_visible(c.oid)",
.namespace = "c.relnamespace",
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index a941f2accd..c30ca01726 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -22,6 +22,7 @@
/* flag bits for ClusterParams->flags */
#define CLUOPT_RECHECK 0x01 /* recheck relation state */
#define CLUOPT_VERBOSE 0x02 /* print progress info */
+#define CLUOPT_RECHECK_ISCLUSTERED 0x04 /* recheck relation state for indisclustered */
/* options for CLUSTER */
typedef struct ClusterParams
diff --git a/src/test/regress/expected/cluster.out b/src/test/regress/expected/cluster.out
index bdae8fe00c..c74cfa88cc 100644
--- a/src/test/regress/expected/cluster.out
+++ b/src/test/regress/expected/cluster.out
@@ -439,14 +439,62 @@ select * from clstr_temp;
drop table clstr_temp;
RESET SESSION AUTHORIZATION;
--- Check that partitioned tables cannot be clustered
+-- Check that partitioned tables can be clustered
CREATE TABLE clstrpart (a int) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart1 PARTITION OF clstrpart FOR VALUES FROM (1)TO(10) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart11 PARTITION OF clstrpart1 FOR VALUES FROM (1)TO(10);
+CREATE TABLE clstrpart12 PARTITION OF clstrpart1 FOR VALUES FROM (10)TO(20) PARTITION BY RANGE(a);
+CREATE TABLE clstrpart2 PARTITION OF clstrpart FOR VALUES FROM (20)TO(30);
+CREATE TABLE clstrpart3 PARTITION OF clstrpart DEFAULT PARTITION BY RANGE(a);
+CREATE TABLE clstrpart33 PARTITION OF clstrpart3 DEFAULT;
+ALTER TABLE clstrpart SET WITHOUT CLUSTER;
+CREATE INDEX clstrpart_only_idx ON ONLY clstrpart (a);
+CLUSTER clstrpart USING clstrpart_only_idx; -- fails
+ERROR: cannot cluster on invalid index "clstrpart_only_idx"
+DROP INDEX clstrpart_only_idx;
CREATE INDEX clstrpart_idx ON clstrpart (a);
-ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
-ERROR: cannot mark index clustered in partitioned table
+-- Check that clustering sets new relfilenodes:
+CREATE TEMP TABLE old_cluster_info AS SELECT relname, level, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ;
CLUSTER clstrpart USING clstrpart_idx;
-ERROR: cannot cluster a partitioned table
-DROP TABLE clstrpart;
+CREATE TEMP TABLE new_cluster_info AS SELECT relname, level, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ;
+SELECT relname, old.level, old.relkind, old.relfilenode = new.relfilenode FROM old_cluster_info AS old JOIN new_cluster_info AS new USING (relname) ORDER BY relname COLLATE "C";
+ relname | level | relkind | ?column?
+-------------+-------+---------+----------
+ clstrpart | 0 | p | t
+ clstrpart1 | 1 | p | t
+ clstrpart11 | 2 | r | f
+ clstrpart12 | 2 | p | t
+ clstrpart2 | 1 | r | f
+ clstrpart3 | 1 | p | t
+ clstrpart33 | 2 | r | f
+(7 rows)
+
+-- Check that clustering sets new indisclustered:
+SELECT relname, relkind, indisclustered FROM pg_partition_tree('clstrpart_idx'::regclass) AS tree JOIN pg_index i ON i.indexrelid=tree.relid JOIN pg_class c ON c.oid=indexrelid ORDER BY relname COLLATE "C";
+ relname | relkind | indisclustered
+-------------------+---------+----------------
+ clstrpart11_a_idx | i | t
+ clstrpart12_a_idx | I | t
+ clstrpart1_a_idx | I | t
+ clstrpart2_a_idx | i | t
+ clstrpart33_a_idx | i | t
+ clstrpart3_a_idx | I | t
+ clstrpart_idx | I | t
+(7 rows)
+
+CLUSTER clstrpart1 USING clstrpart1_a_idx; -- partition which is itself partitioned
+CLUSTER clstrpart12 USING clstrpart12_a_idx; -- partition which is itself partitioned, no childs
+CLUSTER clstrpart2 USING clstrpart2_a_idx; -- leaf
+\d clstrpart
+ Partitioned table "public.clstrpart"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+Partition key: RANGE (a)
+Indexes:
+ "clstrpart_idx" btree (a) CLUSTER
+Number of partitions: 3 (Use \d+ to list them.)
+
-- Test CLUSTER with external tuplesorting
create table clstr_4 as select * from tenk1;
create index cluster_sort on clstr_4 (hundred, thousand, tenthous);
diff --git a/src/test/regress/sql/cluster.sql b/src/test/regress/sql/cluster.sql
index 188183647c..9bcc77695c 100644
--- a/src/test/regress/sql/cluster.sql
+++ b/src/test/regress/sql/cluster.sql
@@ -196,12 +196,30 @@ drop table clstr_temp;
RESET SESSION AUTHORIZATION;
--- Check that partitioned tables cannot be clustered
+-- Check that partitioned tables can be clustered
CREATE TABLE clstrpart (a int) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart1 PARTITION OF clstrpart FOR VALUES FROM (1)TO(10) PARTITION BY RANGE (a);
+CREATE TABLE clstrpart11 PARTITION OF clstrpart1 FOR VALUES FROM (1)TO(10);
+CREATE TABLE clstrpart12 PARTITION OF clstrpart1 FOR VALUES FROM (10)TO(20) PARTITION BY RANGE(a);
+CREATE TABLE clstrpart2 PARTITION OF clstrpart FOR VALUES FROM (20)TO(30);
+CREATE TABLE clstrpart3 PARTITION OF clstrpart DEFAULT PARTITION BY RANGE(a);
+CREATE TABLE clstrpart33 PARTITION OF clstrpart3 DEFAULT;
+ALTER TABLE clstrpart SET WITHOUT CLUSTER;
+CREATE INDEX clstrpart_only_idx ON ONLY clstrpart (a);
+CLUSTER clstrpart USING clstrpart_only_idx; -- fails
+DROP INDEX clstrpart_only_idx;
CREATE INDEX clstrpart_idx ON clstrpart (a);
-ALTER TABLE clstrpart CLUSTER ON clstrpart_idx;
+-- Check that clustering sets new relfilenodes:
+CREATE TEMP TABLE old_cluster_info AS SELECT relname, level, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ;
CLUSTER clstrpart USING clstrpart_idx;
-DROP TABLE clstrpart;
+CREATE TEMP TABLE new_cluster_info AS SELECT relname, level, relfilenode, relkind FROM pg_partition_tree('clstrpart'::regclass) AS tree JOIN pg_class c ON c.oid=tree.relid ;
+SELECT relname, old.level, old.relkind, old.relfilenode = new.relfilenode FROM old_cluster_info AS old JOIN new_cluster_info AS new USING (relname) ORDER BY relname COLLATE "C";
+-- Check that clustering sets new indisclustered:
+SELECT relname, relkind, indisclustered FROM pg_partition_tree('clstrpart_idx'::regclass) AS tree JOIN pg_index i ON i.indexrelid=tree.relid JOIN pg_class c ON c.oid=indexrelid ORDER BY relname COLLATE "C";
+CLUSTER clstrpart1 USING clstrpart1_a_idx; -- partition which is itself partitioned
+CLUSTER clstrpart12 USING clstrpart12_a_idx; -- partition which is itself partitioned, no childs
+CLUSTER clstrpart2 USING clstrpart2_a_idx; -- leaf
+\d clstrpart
-- Test CLUSTER with external tuplesorting
--
2.17.0
--AA9g+nFNFPYNJKiL
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v7-0003-Propagate-changes-to-indisclustered-to-child-pare.patch"
^ permalink raw reply [nested|flat] 5+ messages in thread
* Proposal to Enable/Disable Index using ALTER INDEX
@ 2024-09-09 21:38 Shayon Mukherjee <[email protected]>
2024-09-09 22:16 ` Re: Proposal to Enable/Disable Index using ALTER INDEX David Rowley <[email protected]>
2024-09-23 15:14 ` Re: Proposal to Enable/Disable Index using ALTER INDEX Peter Eisentraut <[email protected]>
0 siblings, 2 replies; 5+ messages in thread
From: Shayon Mukherjee @ 2024-09-09 21:38 UTC (permalink / raw)
To: [email protected]
Hello hackers,
This is my first time posting here, and I’d like to propose a new feature
related to PostgreSQL indexes. If this idea resonates, I’d be happy to
follow up with a patch as well.
*Problem*:
Adding and removing indexes is a common operation in PostgreSQL. On larger
databases, however, these operations can be resource-intensive. When
evaluating the performance impact of one or more indexes, dropping them
might not be ideal since as a user you may want a quicker way to test their
effects without fully committing to removing & adding them back again.
Which can be a time taking operation on larger tables.
*Proposal*:
I propose adding an ALTER INDEX command that allows for enabling or
disabling an index globally. This could look something like:
ALTER INDEX index_name ENABLE;
ALTER INDEX index_name DISABLE;
A disabled index would still receive updates and enforce constraints as
usual but would not be used for queries. This allows users to assess
whether an index impacts query performance before deciding to drop it
entirely.
*Implementation*:
To keep this simple, I suggest toggling the indisvalid flag in pg_index
during the enable/disable operation.
*Additional Considerations*:
- Keeping the index up-to-date while it’s disabled seems preferable, as it
avoids the need to rebuild the index if it’s re-enabled later. The
alternative would be dropping and rebuilding the index upon re-enabling,
which I believe would introduce additional overhead in terms of application
logic & complexity.
- I am also proposing to reuse the existing indisvalid flag to avoid adding
new state and the maintenance that comes with it, but I’m open to feedback
if this approach has potential downsides.
- To keep the scope minimal for now, I propose that we only allow enabling
and disabling indexes globally, and not locally, by supporting it
exclusively in ALTER INDEX. I would love to know if this would break any
SQL grammar promises that I might be unaware of.
I would love to learn if this sounds like a good idea and how it can be
improved further. Accordingly, as a next step I would be very happy to
propose a patch as well.
Best regards,
Shayon Mukherjee
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Proposal to Enable/Disable Index using ALTER INDEX
2024-09-09 21:38 Proposal to Enable/Disable Index using ALTER INDEX Shayon Mukherjee <[email protected]>
@ 2024-09-09 22:16 ` David Rowley <[email protected]>
2024-09-10 15:12 ` Re: Proposal to Enable/Disable Index using ALTER INDEX Nathan Bossart <[email protected]>
1 sibling, 1 reply; 5+ messages in thread
From: David Rowley @ 2024-09-09 22:16 UTC (permalink / raw)
To: Shayon Mukherjee <[email protected]>; +Cc: [email protected]
On Tue, 10 Sept 2024 at 09:39, Shayon Mukherjee <[email protected]> wrote:
> Adding and removing indexes is a common operation in PostgreSQL. On larger databases, however, these operations can be resource-intensive. When evaluating the performance impact of one or more indexes, dropping them might not be ideal since as a user you may want a quicker way to test their effects without fully committing to removing & adding them back again. Which can be a time taking operation on larger tables.
>
> Proposal:
> I propose adding an ALTER INDEX command that allows for enabling or disabling an index globally. This could look something like:
>
> ALTER INDEX index_name ENABLE;
> ALTER INDEX index_name DISABLE;
>
> A disabled index would still receive updates and enforce constraints as usual but would not be used for queries. This allows users to assess whether an index impacts query performance before deciding to drop it entirely.
I personally think having some way to alter an index to stop it from
being used in query plans would be very useful for the reasons you
mentioned. I don't have any arguments against the syntax you've
proposed. We'd certainly have to clearly document that constraints
are still enforced. Perhaps there is some other syntax which would
self-document slightly better. I just can't think of it right now.
> Implementation:
> To keep this simple, I suggest toggling the indisvalid flag in pg_index during the enable/disable operation.
That's not a good idea as it would allow ALTER INDEX ... ENABLE; to be
used to make valid a failed concurrently created index. I think this
would need a new flag and everywhere in the planner would need to be
adjusted to ignore indexes when that flag is false.
> Additional Considerations:
> - Keeping the index up-to-date while it’s disabled seems preferable, as it avoids the need to rebuild the index if it’s re-enabled later. The alternative would be dropping and rebuilding the index upon re-enabling, which I believe would introduce additional overhead in terms of application logic & complexity.
I think the primary use case here is to assist in dropping useless
indexes in a way that can very quickly be undone if the index is more
useful than thought. If you didn't keep the index up-to-date then that
would make the feature useless for that purpose.
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.
David
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Proposal to Enable/Disable Index using ALTER INDEX
2024-09-09 21:38 Proposal to Enable/Disable Index using ALTER INDEX Shayon Mukherjee <[email protected]>
2024-09-09 22:16 ` Re: Proposal to Enable/Disable Index using ALTER INDEX David Rowley <[email protected]>
@ 2024-09-10 15:12 ` Nathan Bossart <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Nathan Bossart @ 2024-09-10 15:12 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: Shayon Mukherjee <[email protected]>; [email protected]
On Tue, Sep 10, 2024 at 10:16:34AM +1200, David Rowley wrote:
> I think the primary use case here is to assist in dropping useless
> indexes in a way that can very quickly be undone if the index is more
> useful than thought. If you didn't keep the index up-to-date then that
> would make the feature useless for that purpose.
>
> 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
--
nathan
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Proposal to Enable/Disable Index using ALTER INDEX
2024-09-09 21:38 Proposal to Enable/Disable Index using ALTER INDEX Shayon Mukherjee <[email protected]>
@ 2024-09-23 15:14 ` Peter Eisentraut <[email protected]>
1 sibling, 0 replies; 5+ messages in thread
From: Peter Eisentraut @ 2024-09-23 15:14 UTC (permalink / raw)
To: Shayon Mukherjee <[email protected]>; [email protected]
On 09.09.24 23:38, Shayon Mukherjee wrote:
> *Problem*:
> Adding and removing indexes is a common operation in PostgreSQL. On
> larger databases, however, these operations can be resource-intensive.
> When evaluating the performance impact of one or more indexes, dropping
> them might not be ideal since as a user you may want a quicker way to
> test their effects without fully committing to removing & adding them
> back again. Which can be a time taking operation on larger tables.
>
> *Proposal*:
> I propose adding an ALTER INDEX command that allows for enabling or
> disabling an index globally. This could look something like:
>
> ALTER INDEX index_name ENABLE;
> ALTER INDEX index_name DISABLE;
>
> A disabled index would still receive updates and enforce constraints as
> usual but would not be used for queries. This allows users to assess
> whether an index impacts query performance before deciding to drop it
> entirely.
I think a better approach would be to make the list of disabled indexes
a GUC setting, which would then internally have an effect similar to
enable_indexscan, meaning it would make the listed indexes unattractive
to the planner.
This seems better than the proposed DDL command, because you'd be able
to use this per-session, instead of forcing a global state, and even
unprivileged users could use it.
(I think we have had proposals like this before, but I can't find the
discussion I'm thinking of right now.)
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-09-23 15:14 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-06-07 21:58 [PATCH v7 2/7] Implement CLUSTER of partitioned table.. Justin Pryzby <[email protected]>
2024-09-09 21:38 Proposal to Enable/Disable Index using ALTER INDEX Shayon Mukherjee <[email protected]>
2024-09-09 22:16 ` Re: Proposal to Enable/Disable Index using ALTER INDEX David Rowley <[email protected]>
2024-09-10 15:12 ` Re: Proposal to Enable/Disable Index using ALTER INDEX Nathan Bossart <[email protected]>
2024-09-23 15:14 ` Re: Proposal to Enable/Disable Index using ALTER INDEX Peter Eisentraut <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox