Subject: [PATCH v7 1/2] ALTER tbl rewrite loses CLUSTER ON index

From: Amit Langote <[email-masked]>
Date: Mon, 16 Mar 2020 16:01:42 +0900

On Fri, Mar 13, 2020 at 2:19 AM Tom Lane <[email-masked]> wrote:
> Justin Pryzby <[email-masked]> writes:
> > @cfbot: resending with only Amit's 0001, since Michael pushed a variation on
> > 0002.

Thank you for taking a look at it.

> Boy, I really dislike this patch.  ATPostAlterTypeParse is documented as
> using the supplied definition string, and nothing else, to reconstruct
> the index.  This breaks that without even the courtesy of documenting
> the breakage.  Moreover, the reason why it's designed like that is to
> avoid requiring the old index objects to still be accessible.  So I'm
> surprised that this hack works at all.  I don't think it would have
> worked at the time the code was first written, and I think it's imposing
> a constraint we'll have problems with (again?) in future.

Okay, so maybe in the middle of ATPostAlterTypeParse() is not a place
to do it, but don't these arguments apply to
RebuildConstraintComment(), which I based the patch on?

> The right way to fix this is to note the presence of the indisclustered
> flag when we're examining the index earlier, and include a suitable
> command in the definition string.  So probably pg_get_indexdef_string()
> is what needs to change.  It doesn't look like that's used anywhere
> else, so we can just redefine its behavior as needed.

I came across a commit that recently went in:

commit 1cc9c2412cc9a2fbe6a381170097d315fd40ccca
Author: Peter Eisentraut <[email-masked]>
Date:   Fri Mar 13 11:28:11 2020 +0100

    Preserve replica identity index across ALTER TABLE rewrite

which fixes something very similar to what we are trying to with this
patch.  The way it's done looks to me very close to what you are
telling.  I have updated the patch to be similar to the above fix.

--
Thank you,
Amit
---
 src/backend/commands/tablecmds.c          | 49 +++++++++++++++++++++++
 src/backend/utils/cache/lsyscache.c       | 23 +++++++++++
 src/include/utils/lsyscache.h             |  1 +
 src/test/regress/expected/alter_table.out | 34 ++++++++++++++++
 src/test/regress/sql/alter_table.sql      | 15 +++++++
 5 files changed, 122 insertions(+)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8c33b67c1b..73d9392878 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 CLUSTER ON */
 } AlteredTableInfo;
 
 /* Struct describing one new constraint to check in Phase 3 scan */
@@ -11579,9 +11580,28 @@ 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).
+ *
+ * For constraint's index (if any), also remember if it's the table's replica
+ * identity or its clustered index, so that ATPostAlterTypeCleanup() can
+ * queue up commands necessary to restore that property.
  */
 static void
 RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab)
@@ -11604,9 +11624,18 @@ RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab)
 		tab->changedConstraintDefs = lappend(tab->changedConstraintDefs,
 											 defstring);
 
+		/*
+		 * For constraint's index (if any), remember if it's the table's
+		 * replica identity or its clustered index, so that
+		 * ATPostAlterTypeCleanup() can queue up commands necessary to restore
+		 * that property.
+		 */
 		indoid = get_constraint_index(conoid);
 		if (OidIsValid(indoid))
+		{
 			RememberReplicaIdentityForRebuilding(indoid, tab);
+			RememberClusterOnForRebuilding(indoid, tab);
+		}
 	}
 }
 
@@ -11650,7 +11679,13 @@ RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab)
 			tab->changedIndexDefs = lappend(tab->changedIndexDefs,
 											defstring);
 
+			/*
+			 * Remember if it's the table's replica identity or its clustered
+			 * index, so that ATPostAlterTypeCleanup() can queue up commands
+			 * necessary to restore that property.
+			 */
 			RememberReplicaIdentityForRebuilding(indoid, tab);
+			RememberClusterOnForRebuilding(indoid, tab);
 		}
 	}
 }
@@ -11777,6 +11812,20 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
 			lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
 	}
 
+	/*
+	 * Queue up command to restore clustered index marking
+	 */
+	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 27bbb58f56..9c5b806c22 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -3299,3 +3299,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/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index 4e646c55e9..9459c6a94f 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -184,6 +184,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/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index fb6d86a269..a01c6d6ec5 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4296,3 +4296,37 @@ create trigger xtrig
 update bar1 set a = a + 1;
 INFO:  a=1, b=1
 /* End test case for bug #16242 */
+-- alter type rewrite/rebuild should preserve cluster marking on index
+create table alttype_cluster (a int);
+create index alttype_cluster_a on alttype_cluster (a);
+alter table alttype_cluster cluster on alttype_cluster_a;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+ indisclustered 
+----------------
+ t
+(1 row)
+
+alter table alttype_cluster alter a type bigint;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+ indisclustered 
+----------------
+ t
+(1 row)
+
+drop index alttype_cluster_a;
+alter table alttype_cluster add primary key (a);
+alter table alttype_cluster cluster on alttype_cluster_pkey;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+ indisclustered 
+----------------
+ t
+(1 row)
+
+alter table alttype_cluster alter a type int;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+ indisclustered 
+----------------
+ t
+(1 row)
+
+drop table alttype_cluster;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 3801f19c58..4eeec24e58 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2840,3 +2840,18 @@ create trigger xtrig
 update bar1 set a = a + 1;
 
 /* End test case for bug #16242 */
+
+-- alter type rewrite/rebuild should preserve cluster marking on index
+create table alttype_cluster (a int);
+create index alttype_cluster_a on alttype_cluster (a);
+alter table alttype_cluster cluster on alttype_cluster_a;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+alter table alttype_cluster alter a type bigint;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+drop index alttype_cluster_a;
+alter table alttype_cluster add primary key (a);
+alter table alttype_cluster cluster on alttype_cluster_pkey;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+alter table alttype_cluster alter a type int;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+drop table alttype_cluster;
-- 
2.17.0

--aZoGpuMECXJckB41
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v7-0002-Use-get_index_isclustered-in-cluster.c.patch"