Hi Justin, On Mon, Feb 3, 2020 at 1:17 AM Justin Pryzby <pry...@telsasoft.com> 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;