Hi Justin,
On Mon, Feb 3, 2020 at 1:17 AM Justin Pryzby <[email protected]> wrote:
> Other options are preserved by ALTER (and CLUSTER ON is and most obviously
> should be preserved by CLUSTER's rewrite), so I think (SET) CLUSTER should be
> preserved by ALTER, too.
Yes.
create table foo (a int primary key);
cluster foo;
ERROR: there is no previously clustered index for table "foo"
cluster foo using foo_pkey;
alter table foo alter a type bigint;
cluster foo;
ERROR: there is no previously clustered index for table "foo"
With your patch, this last error doesn't occur.
Like you, I too suspect that losing indisclustered like this is
unintentional, so should be fixed.
> As far as I can see, this should be the responsibility of something in the
> vicinity of ATPostAlterTypeParse/RememberIndexForRebuilding.
>
> Attach patch sketches a fix.
While your sketch hits pretty close, it could be done a bit
differently. For one, I don't like the way it's misusing
changedIndexOids and changedIndexDefs.
Instead, we can do something similar to what
RebuildConstraintComments() does for constraint comments. For
example, we can have a PreserveClusterOn() that adds a AT_ClusterOn
command into table's AT_PASS_OLD_INDEX pass commands. Attached patch
shows what I'm thinking. I also added representative tests.
Thanks,
Amit
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f599393473..ea9941278e 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,
@@ -11832,6 +11833,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))
{
@@ -11868,6 +11872,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)
{
@@ -11990,6 +11997,37 @@ RebuildConstraintComment(AlteredTableInfo *tab, int
pass, Oid objid,
tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
}
+/*
+ * For an index on table that's being recreated due PostAlterType processing,
+ * preserve its indisclustered property by issuing ALTER TABLE CLUSTER ON on
+ * table to run after the command to re-create 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 e73ce4b6f3..010a7aa98b 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4258,3 +4258,37 @@ alter table at_test_sql_partop attach partition
at_test_sql_partop_1 for values
drop table at_test_sql_partop;
drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
+-- alter type shouldn't lose clustered 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 a16e4c9a29..d619a98de7 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2801,3 +2801,18 @@ alter table at_test_sql_partop attach partition
at_test_sql_partop_1 for values
drop table at_test_sql_partop;
drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
+
+-- alter type shouldn't lose clustered 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;
