On Wed, Mar 18, 2020 at 11:48:37AM +0900, Michael Paquier wrote: > Anyway, Tom, Alvaro, are you planning to look at what is proposed on > this thread? I don't want to step on your toes if that's the case and > it seems to me that the approach taken by the patch is sound, using as > basic fix the addition of an AT_ClusterOn sub-command to the list of > commands to execute when rebuilding the table, ensuring that any > follow-up CLUSTER command will use the correct index.
Hearing nothing, I have been looking at the patches sent upthread, and
did some modifications as per the attached for 0001. The logic looked
fine to me and it is unchanged as you are taking care of normal
indexes as well as constraint indexes. Please note that I have
tweaked some comments, and removed what was on top of
RememberConstraintForRebuilding() as that was just a duplicate.
Regarding the tests, I was annoyed by the fact that the logic was not
manipulating two indexes at the same time on the relation rewritten
with a normal and a constraint index, and cross-checking both at the
same time to see which one is clustered after each rewrite is good for
consistency.
Now, regarding patch 0002, note that you have a problem for this part:
- tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
- if (!HeapTupleIsValid(tuple)) /* probably can't happen */
- {
- relation_close(OldHeap, AccessExclusiveLock);
- pgstat_progress_end_command();
- return;
- }
- indexForm = (Form_pg_index) GETSTRUCT(tuple);
- if (!indexForm->indisclustered)
+ if (!get_index_isclustered(indexOid))
{
- ReleaseSysCache(tuple);
relation_close(OldHeap, AccessExclusiveLock);
pgstat_progress_end_command();
return;
}
- ReleaseSysCache(tuple);
On an invalid tuple for pg_index, the new code would issue an error,
while the old code would just return. And it seems to me that this
can lead to problems because the parent relation is processed and
locked at the beginning of cluster_rel(), *after* we know the index
OID to work on. The refactoring is fine for the other two areas
though, so I think that there is still value to apply
get_index_isclustered() within mark_index_clustered() and cluster(),
and I would suggest to keep 0002 to that.
Justin, what do you think?
--
Michael
From 60a2e5a615a39b852985166531019ac1baa4146a Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 2 Apr 2020 14:52:15 +0900
Subject: [PATCH v8] Preserve clustered indexes after rewrites of ALTER TABLE
---
src/include/utils/lsyscache.h | 1 +
src/backend/commands/tablecmds.c | 47 ++++++++++++++++++++++
src/backend/utils/cache/lsyscache.c | 23 +++++++++++
src/test/regress/expected/alter_table.out | 48 +++++++++++++++++++++++
src/test/regress/sql/alter_table.sql | 25 ++++++++++++
5 files changed, 144 insertions(+)
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index 374f57fb43..c9c68e2f4f 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -185,6 +185,7 @@ extern Oid get_range_collation(Oid rangeOid);
extern Oid get_index_column_opclass(Oid index_oid, int attno);
extern bool get_index_isreplident(Oid index_oid);
extern bool get_index_isvalid(Oid index_oid);
+extern bool get_index_isclustered(Oid index_oid);
#define type_is_array(typid) (get_element_type(typid) != InvalidOid)
/* type_is_array_domain accepts both plain arrays and domains over arrays */
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c8c88be2c9..ef234075dc 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -177,6 +177,7 @@ typedef struct AlteredTableInfo
List *changedIndexOids; /* OIDs of indexes to rebuild */
List *changedIndexDefs; /* string definitions of same */
char *replicaIdentityIndex; /* index to reset as REPLICA IDENTITY */
+ char *clusterOnIndex; /* index to use for CLUSTER */
} AlteredTableInfo;
/* Struct describing one new constraint to check in Phase 3 scan */
@@ -11581,6 +11582,21 @@ RememberReplicaIdentityForRebuilding(Oid indoid, AlteredTableInfo *tab)
tab->replicaIdentityIndex = get_rel_name(indoid);
}
+/*
+ * Subroutine for ATExecAlterColumnType: remember any clustered index.
+ */
+static void
+RememberClusterOnForRebuilding(Oid indoid, AlteredTableInfo *tab)
+{
+ if (!get_index_isclustered(indoid))
+ return;
+
+ if (tab->clusterOnIndex)
+ elog(ERROR, "relation %u has multiple clustered indexes", tab->relid);
+
+ tab->clusterOnIndex = get_rel_name(indoid);
+}
+
/*
* Subroutine for ATExecAlterColumnType: remember that a constraint needs
* to be rebuilt (which we might already know).
@@ -11606,9 +11622,18 @@ RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab)
tab->changedConstraintDefs = lappend(tab->changedConstraintDefs,
defstring);
+ /*
+ * For the index of a constraint, if any, remember if it is
+ * used for the table's replica identity or if it is a
+ * clustered index, so that ATPostAlterTypeCleanup() can queue
+ * up commands necessary to restore those properties.
+ */
indoid = get_constraint_index(conoid);
if (OidIsValid(indoid))
+ {
RememberReplicaIdentityForRebuilding(indoid, tab);
+ RememberClusterOnForRebuilding(indoid, tab);
+ }
}
}
@@ -11652,7 +11677,14 @@ RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab)
tab->changedIndexDefs = lappend(tab->changedIndexDefs,
defstring);
+ /*
+ * Remember if this index is used for the table's replica
+ * identity or if it is a clustered index, so that
+ * ATPostAlterTypeCleanup() can queue up commands necessary to
+ * restore those properties.
+ */
RememberReplicaIdentityForRebuilding(indoid, tab);
+ RememberClusterOnForRebuilding(indoid, tab);
}
}
}
@@ -11779,6 +11811,21 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
}
+ /*
+ * Queue up command to restore marking of index used for cluster.
+ */
+ if (tab->clusterOnIndex)
+ {
+ AlterTableCmd *cmd = makeNode(AlterTableCmd);
+
+ cmd->subtype = AT_ClusterOn;
+ cmd->name = tab->clusterOnIndex;
+
+ /* do it after indexes and constraints */
+ tab->subcmds[AT_PASS_OLD_CONSTR] =
+ lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
+ }
+
/*
* It should be okay to use DROP_RESTRICT here, since nothing else should
* be depending on these objects.
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 0a6db0d478..a7d63f107f 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -3334,3 +3334,26 @@ get_index_isvalid(Oid index_oid)
return isvalid;
}
+
+/*
+ * get_index_isclustered
+ *
+ * Given the index OID, return pg_index.indisclustered.
+ */
+bool
+get_index_isclustered(Oid index_oid)
+{
+ bool isclustered;
+ HeapTuple tuple;
+ Form_pg_index rd_index;
+
+ tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(index_oid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for index %u", index_oid);
+
+ rd_index = (Form_pg_index) GETSTRUCT(tuple);
+ isclustered = rd_index->indisclustered;
+ ReleaseSysCache(tuple);
+
+ return isclustered;
+}
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index fb6d86a269..81a8e5fc5a 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4296,3 +4296,51 @@ create trigger xtrig
update bar1 set a = a + 1;
INFO: a=1, b=1
/* End test case for bug #16242 */
+-- Test that ALTER TABLE rewrite preserves a clustered index
+-- for normal indexes and indexes on constraints.
+create table alttype_cluster (a int);
+alter table alttype_cluster add primary key (a);
+create index alttype_cluster_ind on alttype_cluster (a);
+alter table alttype_cluster cluster on alttype_cluster_ind;
+-- Normal index remains clustered.
+select indexrelid::regclass, indisclustered from pg_index
+ where indrelid = 'alttype_cluster'::regclass
+ order by indexrelid::regclass::text;
+ indexrelid | indisclustered
+----------------------+----------------
+ alttype_cluster_ind | t
+ alttype_cluster_pkey | f
+(2 rows)
+
+alter table alttype_cluster alter a type bigint;
+select indexrelid::regclass, indisclustered from pg_index
+ where indrelid = 'alttype_cluster'::regclass
+ order by indexrelid::regclass::text;
+ indexrelid | indisclustered
+----------------------+----------------
+ alttype_cluster_ind | t
+ alttype_cluster_pkey | f
+(2 rows)
+
+-- Constraint index remains clustered.
+alter table alttype_cluster cluster on alttype_cluster_pkey;
+select indexrelid::regclass, indisclustered from pg_index
+ where indrelid = 'alttype_cluster'::regclass
+ order by indexrelid::regclass::text;
+ indexrelid | indisclustered
+----------------------+----------------
+ alttype_cluster_ind | f
+ alttype_cluster_pkey | t
+(2 rows)
+
+alter table alttype_cluster alter a type int;
+select indexrelid::regclass, indisclustered from pg_index
+ where indrelid = 'alttype_cluster'::regclass
+ order by indexrelid::regclass::text;
+ indexrelid | indisclustered
+----------------------+----------------
+ alttype_cluster_ind | f
+ alttype_cluster_pkey | t
+(2 rows)
+
+drop table alttype_cluster;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 3801f19c58..5e6b74c54f 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2840,3 +2840,28 @@ create trigger xtrig
update bar1 set a = a + 1;
/* End test case for bug #16242 */
+
+-- Test that ALTER TABLE rewrite preserves a clustered index
+-- for normal indexes and indexes on constraints.
+create table alttype_cluster (a int);
+alter table alttype_cluster add primary key (a);
+create index alttype_cluster_ind on alttype_cluster (a);
+alter table alttype_cluster cluster on alttype_cluster_ind;
+-- Normal index remains clustered.
+select indexrelid::regclass, indisclustered from pg_index
+ where indrelid = 'alttype_cluster'::regclass
+ order by indexrelid::regclass::text;
+alter table alttype_cluster alter a type bigint;
+select indexrelid::regclass, indisclustered from pg_index
+ where indrelid = 'alttype_cluster'::regclass
+ order by indexrelid::regclass::text;
+-- Constraint index remains clustered.
+alter table alttype_cluster cluster on alttype_cluster_pkey;
+select indexrelid::regclass, indisclustered from pg_index
+ where indrelid = 'alttype_cluster'::regclass
+ order by indexrelid::regclass::text;
+alter table alttype_cluster alter a type int;
+select indexrelid::regclass, indisclustered from pg_index
+ where indrelid = 'alttype_cluster'::regclass
+ order by indexrelid::regclass::text;
+drop table alttype_cluster;
--
2.26.0
signature.asc
Description: PGP signature
