[Justin Pryzby <pryzby@telsasoft.com>]

On Fri, Feb 28, 2020 at 08:42:02PM -0600, Justin Pryzby wrote:
> On Fri, Feb 28, 2020 at 06:26:04PM -0500, Tom Lane wrote:
> > Justin Pryzby <pryzby@telsasoft.com> writes:
> > > I think the attached is 80% complete (I didn't touch pg_dump).
> > > One objection to this change would be that all relations (including indices)
> > > end up with relclustered fields, and pg_index already has a number of bools, so
> > > it's not like this one bool is wasting a byte.
> > > I think relisclustered was a's clever way of avoiding that overhead (c0ad5953).
> > > So I would be -0.5 on moving it to pg_class..
> > > But I think 0001 and 0002 are worthy.  Maybe the test in 0002 should live
> > > somewhere else.
> > 
> > 0001 has been superseded by events (faade5d4c), so the cfbot is choking
> > on that one's failure to apply, and not testing any further.  Please
> > repost without 0001 so that we can get this testing again.
> 
> I've just noticed while working on (1) that this separately affects REINDEX
> CONCURRENTLY, which would be a new bug in v12.  Without CONCURRENTLY there's no
> issue.  I guess we need a separate patch for that case.

Rebased Amit's patch and included my own 0002 to fix the REINDEX CONCURRENTLY
issue.

-- 
Justin

Attachments:

  [text/x-diff] v4-0001-ALTER-tbl-rewrite-loses-CLUSTER-ON-index.patch (5.8K, ../20200229165258.GJ29456@telsasoft.com/2-v4-0001-ALTER-tbl-rewrite-loses-CLUSTER-ON-index.patch)
  download | inline diff:
From dccc6f08450eacafc3a08bc8b2836d2feb23a533 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlangote09@gmail.com>
Date: Thu, 6 Feb 2020 18:14:16 +0900
Subject: [PATCH v4 1/2] ALTER tbl rewrite loses CLUSTER ON index

---
 src/backend/commands/tablecmds.c          | 39 +++++++++++++++++++++++
 src/test/regress/expected/alter_table.out | 34 ++++++++++++++++++++
 src/test/regress/sql/alter_table.sql      | 16 +++++++++-
 3 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 02a7c04fdb..6b2469bd09 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -490,6 +490,7 @@ static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
 static void RebuildConstraintComment(AlteredTableInfo *tab, int pass,
 									 Oid objid, Relation rel, List *domname,
 									 const char *conname);
+static void PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid);
 static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
 static void TryReuseForeignKey(Oid oldId, Constraint *con);
 static ObjectAddress ATExecAlterColumnGenericOptions(Relation rel, const char *colName,
@@ -11838,6 +11839,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
 			newcmd->def = (Node *) stmt;
 			tab->subcmds[AT_PASS_OLD_INDEX] =
 				lappend(tab->subcmds[AT_PASS_OLD_INDEX], newcmd);
+
+			/* Preserve index's indisclustered property, if set. */
+			PreserveClusterOn(tab, AT_PASS_OLD_INDEX, oldId);
 		}
 		else if (IsA(stm, AlterTableStmt))
 		{
@@ -11874,6 +11878,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
 											 rel,
 											 NIL,
 											 indstmt->idxname);
+
+					/* Preserve index's indisclustered property, if set. */
+					PreserveClusterOn(tab, AT_PASS_OLD_INDEX, indoid);
 				}
 				else if (cmd->subtype == AT_AddConstraint)
 				{
@@ -11996,6 +12003,38 @@ RebuildConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
 	tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
 }
 
+/*
+ * For a table's index that is to be recreated due to PostAlterType
+ * processing, preserve its indisclustered property by issuing ALTER TABLE
+ * CLUSTER ON command on the table that will run after the command to recreate
+ * the index.
+ */
+static void
+PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid)
+{
+	HeapTuple	indexTuple;
+	Form_pg_index indexForm;
+
+	Assert(OidIsValid(indoid));
+	Assert(pass == AT_PASS_OLD_INDEX);
+
+	indexTuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indoid));
+	if (!HeapTupleIsValid(indexTuple))
+		elog(ERROR, "cache lookup failed for index %u", indoid);
+	indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
+
+	if (indexForm->indisclustered)
+	{
+		AlterTableCmd *newcmd = makeNode(AlterTableCmd);
+
+		newcmd->subtype = AT_ClusterOn;
+		newcmd->name = get_rel_name(indoid);
+		tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
+	}
+
+	ReleaseSysCache(indexTuple);
+}
+
 /*
  * Subroutine for ATPostAlterTypeParse().  Calls out to CheckIndexCompatible()
  * for the real analysis, then mutates the IndexStmt based on that verdict.
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..6e9048bbec 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2802,7 +2802,6 @@ drop table at_test_sql_partop;
 drop operator class at_test_sql_partop using btree;
 drop function at_test_sql_partop;
 
-
 /* Test case for bug #16242 */
 
 -- We create a parent and child where the child has missing
@@ -2840,3 +2839,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

  [text/x-diff] v4-0002-CREATE-INDEX-CONCURRENTLY-to-preserve-CLUSTER.patch (3.1K, ../20200229165258.GJ29456@telsasoft.com/3-v4-0002-CREATE-INDEX-CONCURRENTLY-to-preserve-CLUSTER.patch)
  download | inline diff:
From 855344f709a02907a81ccbe950cd99864b1d2030 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 29 Feb 2020 10:38:18 -0600
Subject: [PATCH v4 2/2] CREATE INDEX CONCURRENTLY to preserve CLUSTER..

---
 src/backend/catalog/index.c                |  5 ++++-
 src/test/regress/expected/create_index.out | 12 ++++++++++++
 src/test/regress/sql/create_index.sql      |  8 ++++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 8880586c37..1d0b1a29aa 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1527,10 +1527,13 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
 	newIndexForm->indimmediate = oldIndexForm->indimmediate;
 	oldIndexForm->indimmediate = true;
 
+	/* Preserve indisclustered */
+	newIndexForm->indisclustered = oldIndexForm->indisclustered;
+	oldIndexForm->indisclustered = false;
+
 	/* Mark old index as valid and new as invalid as index_set_state_flags */
 	newIndexForm->indisvalid = true;
 	oldIndexForm->indisvalid = false;
-	oldIndexForm->indisclustered = false;
 
 	CatalogTupleUpdate(pg_index, &oldIndexTuple->t_self, oldIndexTuple);
 	CatalogTupleUpdate(pg_index, &newIndexTuple->t_self, newIndexTuple);
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 6ddf3a63c3..2f0776bfd9 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2128,6 +2128,18 @@ SELECT obj_description('testcomment_idx1'::regclass, 'pg_class');
 (1 row)
 
 DROP TABLE testcomment;
+-- Check that CLUSTER ON is preserved
+CREATE TABLE concur_clustered(i int);
+CREATE INDEX concur_clustered_i_idx ON concur_clustered(i);
+ALTER TABLE concur_clustered CLUSTER ON concur_clustered_i_idx;
+REINDEX INDEX CONCURRENTLY concur_clustered_i_idx;
+SELECT indexrelid::regclass FROM pg_index WHERE indrelid='concur_clustered'::regclass;
+       indexrelid       
+------------------------
+ concur_clustered_i_idx
+(1 row)
+
+DROP TABLE concur_clustered;
 -- Partitions
 -- Create some partitioned tables
 CREATE TABLE concur_reindex_part (c1 int, c2 int) PARTITION BY RANGE (c1);
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index f7fd756189..73f7ff395c 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -858,6 +858,14 @@ SELECT obj_description('testcomment_idx1'::regclass, 'pg_class');
 REINDEX TABLE CONCURRENTLY testcomment ;
 SELECT obj_description('testcomment_idx1'::regclass, 'pg_class');
 DROP TABLE testcomment;
+-- Check that CLUSTER ON is preserved
+CREATE TABLE concur_clustered(i int);
+CREATE INDEX concur_clustered_i_idx ON concur_clustered(i);
+ALTER TABLE concur_clustered CLUSTER ON concur_clustered_i_idx;
+REINDEX INDEX CONCURRENTLY concur_clustered_i_idx;
+SELECT indexrelid::regclass FROM pg_index WHERE indrelid='concur_clustered'::regclass;
+DROP TABLE concur_clustered;
+
 -- Partitions
 -- Create some partitioned tables
 CREATE TABLE concur_reindex_part (c1 int, c2 int) PARTITION BY RANGE (c1);
-- 
2.17.0