On 2019/04/25 19:02, Amit Langote wrote:
> On 2019/04/25 11:21, Amit Langote wrote:
>> On 2019/04/25 8:27, Tom Lane wrote:
>>> BTW, I hadn't ever looked closely at what the index reuse code
>>> does, and now that I have, my heart just sinks. I think that
>>> logic needs to be nuked from orbit. RelationPreserveStorage was
>>> never meant to be abused in this way; I invite you to contemplate
>>> whether it's not a problem that it doesn't check the backend and
>>> nestLevel fields of PendingRelDelete entries before killing them.
>>> (In its originally-designed use for mapped rels at transaction end,
>>> that wasn't a problem, but I'm afraid that it may be one now.)
>>>
>>> The right way to do this IMO would be something closer to the
>>> heap-swap logic in cluster.c, where we exchange the relfilenodes
>>> of two live indexes, rather than what is happening now. Or for
>>> that matter, do we really need to delete the old indexes at all?
>>
>> Yeah, we wouldn't need TryReuseIndex and subsequent
>> RelationPreserveStorage if we hadn't dropped the old indexes to begin
>> with. Instead, in ATPostAlterTypeParse, check if the index after ALTER
>> TYPE is still incompatible (CheckIndexCompatible) and if it is, don't add
>> a new AT_ReAddIndex command. If it's not, *then* drop the index, and
>> recreate the index from scratch using an IndexStmt generated from the old
>> index definition. I guess We can get rid of IndexStmt.oldNode too.
>
> Thinking on this more and growing confident that we could indeed avoid
> drop index + recreate-it-while-preserving-storage, instead by just not
> doing anything when CheckIndexCompatible says the old index will be fine
> despite ALTER TYPE, but only if the table is not rewritten. I gave this a
> try and came up with the attached patch. It fixes the bug related to
> partitioned indexes (the originally reported one) and then some.
>
> Basically, I aimed to rewrite the code in ATPostAlterTypeCleanup and
> ATPostAlterTypeParse such that we no longer have to rely on an
> implementation based on setting "oldNode" to preserve old indexes. With
> the attached, for the cases in which the table won't be rewritten and
> hence the indexes not rebuilt, ATPostAlterTypeParse() simply won't queue a
> AT_ReAddIndex command to rebuild index while preserving the storage. That
> means both ATAddIndex() and DefineIndex can be freed of the duty of
> looking out for the "oldNode" case, because that case no longer exists.
>
> Another main change is that inherited (!conislocal) constraints are now
> recognized by ATPostAlterTypeParse directly, instead of
> ATPostAlterTypeCleanup checking for them and skipping
> ATPostAlterTypeParse() as a whole for such constraints. For one, I had to
> make that change to make the above-described approach work. Also, doing
> that allowed to fix another bug whereby the comments of child constraints
> would go away when they're reconstructed. Notice what happens on
> un-patched PG 11:
>
> create table pp (a int, b text, unique (a, b), c varchar(64)) partition by
> list (a);
> create table pp1 partition of pp for values in (1);
> create table pp2 partition of pp for values in (2);
> alter table pp add constraint c_chk check (c <> '');
> comment on constraint c_chk ON pp is 'parent check constraint';
> comment on constraint c_chk ON pp1 is 'child check constraint 1';
> comment on constraint c_chk ON pp2 is 'child check constraint 2';
> select conname, obj_description(oid, 'pg_constraint') from pg_constraint
> where conname = 'c_chk';
> conname │ obj_description
> ─────────┼──────────────────────────
> c_chk │ parent check constraint
> c_chk │ child check constraint 1
> c_chk │ child check constraint 2
> (3 rows)
>
> alter table pp alter c type varchar(64);
>
> select conname, obj_description(oid, 'pg_constraint') from pg_constraint
> where conname = 'c_chk';
> conname │ obj_description
> ─────────┼─────────────────────────
> c_chk │ parent check constraint
> c_chk │
> c_chk │
> (3 rows)
>
> The patch fixes that with some surgery of RebuildConstraintComment
> combined with aforementioned changes. With the patch:
>
> alter table pp alter c type varchar(64);
>
> select conname, obj_description(oid, 'pg_constraint') from pg_constraint
> where conname = 'c_chk';
> conname │ obj_description
> ─────────┼──────────────────────────
> c_chk │ parent check constraint
> c_chk │ child check constraint 1
> c_chk │ child check constraint 2
> (3 rows)
>
> alter table pp alter c type varchar(128);
>
> select conname, obj_description(oid, 'pg_constraint') from pg_constraint
> where conname = 'c_chk';
> conname │ obj_description
> ─────────┼──────────────────────────
> c_chk │ parent check constraint
> c_chk │ child check constraint 1
> c_chk │ child check constraint 2
> (3 rows)
>
> Also for index comments, but only in the case when indexes are not rebuilt.
>
> comment on index pp_a_b_key is 'parent index';
> comment on index pp1_a_b_key is 'child index 1';
> comment on index pp2_a_b_key is 'child index 2';
>
> select relname, relfilenode, obj_description(oid,'pg_class') from pg_class
> where relname like 'pp%key';
> relname │ relfilenode │ obj_description
> ─────────────┼─────────────┼─────────────────
> pp1_a_b_key │ 17280 │ child index 1
> pp2_a_b_key │ 17284 │ child index 2
> pp_a_b_key │ 17271 │ parent index
> (3 rows)
>
> -- no rewrite, indexes untouched, comments preserved
> alter table pp alter b type varchar(128);
>
> select relname, relfilenode, obj_description(oid,'pg_class') from pg_class
> where relname like 'pp%key';
> relname │ relfilenode │ obj_description
> ─────────────┼─────────────┼─────────────────
> pp1_a_b_key │ 17280 │ child index 1
> pp2_a_b_key │ 17284 │ child index 2
> pp_a_b_key │ 17271 │ parent index
> (3 rows)
>
> -- table rewritten, indexes rebuild, child indexes' comments gone
> alter table pp alter b type varchar(64);
>
> select relname, relfilenode, obj_description(oid,'pg_class') from pg_class
> where relname like 'pp%key';
> relname │ relfilenode │ obj_description
> ─────────────┼─────────────┼─────────────────
> pp1_a_b_key │ 17294 │
> pp2_a_b_key │ 17298 │
> pp_a_b_key │ 17285 │ parent index
> (3 rows)
>
>
> I've also added tests for both the originally reported bug and the comment
> ones.
>
> The patch applies to PG 11.
Per Alvaro's report, regression tests added weren't portable. Fixed that
in the attached updated patch.
Thanks,
Amit
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index f8ee4b0a84..64bf2fc932 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -809,11 +809,9 @@ DefineIndex(Oid relationId,
indexRelationName,
RelationGetRelationName(rel))));
}
- /*
- * A valid stmt->oldNode implies that we already have a built form of
the
- * index. The caller should also decline any index build.
- */
- Assert(!OidIsValid(stmt->oldNode) || (skip_build && !stmt->concurrent));
+
+ /* stmt->oldNode is no longer used. */
+ Assert(!OidIsValid(stmt->oldNode));
/*
* Make the catalog entries for the index, including constraints. This
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 65ede339f2..5048e464d5 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -431,11 +431,10 @@ static void ATPostAlterTypeCleanup(List **wqueue,
AlteredTableInfo *tab,
LOCKMODE lockmode);
static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
char *cmd, List **wqueue, LOCKMODE
lockmode,
- bool rewrite);
-static void RebuildConstraintComment(AlteredTableInfo *tab, int pass,
- Oid objid, Relation rel, List
*domname,
- const char *conname);
-static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
+ bool rewrite, bool old_inherited, bool
*drop_old);
+static void RebuildComment(AlteredTableInfo *tab, int pass,
+ Oid objid, Oid classId,
Relation rel, List *domname,
+ const char *conname, const
char *idxname);
static void TryReuseForeignKey(Oid oldId, Constraint *con);
static void change_owner_fix_column_acls(Oid relationOid,
Oid oldOwnerId, Oid
newOwnerId);
@@ -6993,10 +6992,13 @@ ATExecAddIndex(AlteredTableInfo *tab, Relation rel,
/* The IndexStmt has already been through transformIndexStmt */
Assert(stmt->transformed);
+ /* stmt->oldNode is no longer used. */
+ Assert(!OidIsValid(stmt->oldNode));
+
/* suppress schema rights check when rebuilding existing index */
check_rights = !is_rebuild;
- /* skip index build if phase 3 will do it or we're reusing an old one */
- skip_build = tab->rewrite > 0 || OidIsValid(stmt->oldNode);
+ /* skip index build if phase 3 will do it */
+ skip_build = tab->rewrite > 0;
/* suppress notices when rebuilding existing index */
quiet = is_rebuild;
@@ -7011,20 +7013,6 @@ ATExecAddIndex(AlteredTableInfo *tab, Relation rel,
skip_build,
quiet);
- /*
- * If TryReuseIndex() stashed a relfilenode for us, we used it for the
new
- * index instead of building from scratch. The DROP of the old edition
of
- * this index will have scheduled the storage for deletion at commit, so
- * cancel that pending deletion.
- */
- if (OidIsValid(stmt->oldNode))
- {
- Relation irel = index_open(address.objectId, NoLock);
-
- RelationPreserveStorage(irel->rd_node, true);
- index_close(irel, NoLock);
- }
-
return address;
}
@@ -10394,6 +10382,7 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo
*tab, LOCKMODE lockmode)
Oid confrelid;
char contype;
bool conislocal;
+ bool drop_old;
tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(oldId));
if (!HeapTupleIsValid(tup)) /* should not happen */
@@ -10413,18 +10402,6 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo
*tab, LOCKMODE lockmode)
conislocal = con->conislocal;
ReleaseSysCache(tup);
- ObjectAddressSet(obj, ConstraintRelationId, oldId);
- add_exact_object_address(&obj, objects);
-
- /*
- * If the constraint is inherited (only), we don't want to
inject a
- * new definition here; it'll get recreated when
ATAddCheckConstraint
- * recurses from adding the parent table's constraint. But we
had to
- * carry the info this far so that we can drop the constraint
below.
- */
- if (!conislocal)
- continue;
-
/*
* When rebuilding an FK constraint that references the table
we're
* modifying, we might not yet have any lock on the FK's table,
so get
@@ -10434,23 +10411,46 @@ ATPostAlterTypeCleanup(List **wqueue,
AlteredTableInfo *tab, LOCKMODE lockmode)
if (relid != tab->relid && contype == CONSTRAINT_FOREIGN)
LockRelationOid(relid, AccessExclusiveLock);
+ /*
+ * If the old constraint is inherited (!conislocal), it's not
necessary
+ * to inject a new definition, because it will get recreated
when
+ * ATAddCheckConstraint (or ATAddForeignKeyConstraint only in
the case
+ * of partitioning) recurses from adding the parent's
constraint.
+ */
ATPostAlterTypeParse(oldId, relid, confrelid,
(char *)
lfirst(def_item),
- wqueue, lockmode,
tab->rewrite);
+ wqueue, lockmode,
tab->rewrite,
+ !conislocal,
&drop_old);
+
+ if (drop_old)
+ {
+ ObjectAddressSet(obj, ConstraintRelationId, oldId);
+ add_exact_object_address(&obj, objects);
+ }
}
forboth(oid_item, tab->changedIndexOids,
def_item, tab->changedIndexDefs)
{
Oid oldId = lfirst_oid(oid_item);
Oid relid;
+ bool drop_old;
relid = IndexGetRelation(oldId, false);
+ /*
+ * If the old index is a partition, it's not necessary to
inject a new
+ * definition, because it will get recreated when DefineIndex
recurses
+ * from adding the parent's index.
+ */
ATPostAlterTypeParse(oldId, relid, InvalidOid,
(char *)
lfirst(def_item),
- wqueue, lockmode,
tab->rewrite);
+ wqueue, lockmode,
tab->rewrite,
+
get_rel_relispartition(oldId), &drop_old);
- ObjectAddressSet(obj, RelationRelationId, oldId);
- add_exact_object_address(&obj, objects);
+ if (drop_old)
+ {
+ ObjectAddressSet(obj, RelationRelationId, oldId);
+ add_exact_object_address(&obj, objects);
+ }
}
/*
@@ -10469,7 +10469,8 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo
*tab, LOCKMODE lockmode)
static void
ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
- List **wqueue, LOCKMODE lockmode, bool
rewrite)
+ List **wqueue, LOCKMODE lockmode, bool
rewrite,
+ bool old_inherited, bool *drop_old)
{
List *raw_parsetree_list;
List *querytree_list;
@@ -10526,16 +10527,46 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid
refRelId, char *cmd,
IndexStmt *stmt = (IndexStmt *) stm;
AlterTableCmd *newcmd;
- if (!rewrite)
- TryReuseIndex(oldId, stmt);
- /* keep the index's comment */
- stmt->idxcomment = GetComment(oldId,
RelationRelationId, 0);
+ /*
+ * If we won't rewrite the table and the old index is
compatible
+ * even after the ALTER TYPE, we can skip rebuilding
the index.
+ */
+ if (!rewrite &&
+ CheckIndexCompatible(oldId,
+
stmt->accessMethod,
+
stmt->indexParams,
+
stmt->excludeOpNames))
+ {
+ *drop_old = false;
+ continue;
+ }
- newcmd = makeNode(AlterTableCmd);
- newcmd->subtype = AT_ReAddIndex;
- newcmd->def = (Node *) stmt;
- tab->subcmds[AT_PASS_OLD_INDEX] =
- lappend(tab->subcmds[AT_PASS_OLD_INDEX],
newcmd);
+ *drop_old = true;
+
+ /*
+ * If the old index was inherited to begin with,
+ * DefineIndex() will recreate it when rebuilding the
+ * parent index. Although to keep the old comment on
+ * the child index, emit a command to re-add it.
+ */
+ if (!old_inherited)
+ {
+ newcmd = makeNode(AlterTableCmd);
+ newcmd->subtype = AT_ReAddIndex;
+ newcmd->def = (Node *) stmt;
+ tab->subcmds[AT_PASS_OLD_INDEX] =
+
lappend(tab->subcmds[AT_PASS_OLD_INDEX], newcmd);
+ }
+
+ /* recreate any comment on the index */
+ RebuildComment(tab,
+ AT_PASS_OLD_INDEX,
+ oldId,
+ RelationRelationId,
+ rel,
+ NIL,
+ NULL,
+ stmt->idxname);
}
else if (IsA(stm, AlterTableStmt))
{
@@ -10554,44 +10585,94 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid
refRelId, char *cmd,
indstmt = castNode(IndexStmt, cmd->def);
indoid = get_constraint_index(oldId);
- if (!rewrite)
- TryReuseIndex(indoid, indstmt);
- /* keep any comment on the index */
- indstmt->idxcomment = GetComment(indoid,
-
RelationRelationId, 0);
+ /*
+ * If we won't rewrite the table and
the old index is
+ * compatible even after the ALTER
TYPE, we can skip
+ * rebuilding the index.
+ */
+ if (!rewrite &&
+ CheckIndexCompatible(indoid,
+
indstmt->accessMethod,
+
indstmt->indexParams,
+
indstmt->excludeOpNames))
+ {
+ *drop_old = false;
+ continue;
+ }
- cmd->subtype = AT_ReAddIndex;
- tab->subcmds[AT_PASS_OLD_INDEX] =
-
lappend(tab->subcmds[AT_PASS_OLD_INDEX], cmd);
+ *drop_old = true;
- /* recreate any comment on the
constraint */
- RebuildConstraintComment(tab,
-
AT_PASS_OLD_INDEX,
-
oldId,
-
rel,
-
NIL,
-
indstmt->idxname);
+ /*
+ * If the old index was inherited to
begin with,
+ * DefineIndex() will recreate it when
rebuilding the
+ * parent index. Although to keep the
old comment on
+ * the child index, emit a command to
re-add it.
+ */
+ if (!old_inherited)
+ {
+ cmd->subtype = AT_ReAddIndex;
+ tab->subcmds[AT_PASS_OLD_INDEX]
=
+
lappend(tab->subcmds[AT_PASS_OLD_INDEX], cmd);
+ }
+
+ /* recreate any comment on the index
and the constraint */
+ RebuildComment(tab,
+
AT_PASS_OLD_INDEX,
+ indoid,
+
RelationRelationId,
+ rel,
+ NIL,
+ NULL,
+
indstmt->idxname);
+ RebuildComment(tab,
+
AT_PASS_OLD_INDEX,
+ oldId,
+
ConstraintRelationId,
+ rel,
+ NIL,
+
indstmt->idxname,
+ NULL);
}
else if (cmd->subtype == AT_AddConstraint)
{
Constraint *con = castNode(Constraint,
cmd->def);
- con->old_pktable_oid = refRelId;
- /* rewriting neither side of a FK */
- if (con->contype == CONSTR_FOREIGN &&
- !rewrite && tab->rewrite == 0)
- TryReuseForeignKey(oldId, con);
- cmd->subtype = AT_ReAddConstraint;
- tab->subcmds[AT_PASS_OLD_CONSTR] =
-
lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
+ if (!rewrite)
+ {
+ *drop_old = false;
+ continue;
+ }
+
+ *drop_old = true;
+
+ /*
+ * If the old index was inherited to
begin with,
+ * ATExecAddConstraint will recreate it
when rebuilding
+ * the parent constraint. Although to
keep any old
+ * comment on the child constraint,
emit a command to
+ * re-add it.
+ */
+ if (!old_inherited)
+ {
+ con->old_pktable_oid = refRelId;
+ /* rewriting neither side of a
FK */
+ if (con->contype ==
CONSTR_FOREIGN &&
+ !rewrite &&
tab->rewrite == 0)
+
TryReuseForeignKey(oldId, con);
+ cmd->subtype =
AT_ReAddConstraint;
+
tab->subcmds[AT_PASS_OLD_CONSTR] =
+
lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
+ }
/* recreate any comment on the
constraint */
- RebuildConstraintComment(tab,
-
AT_PASS_OLD_CONSTR,
-
oldId,
-
rel,
-
NIL,
-
con->conname);
+ RebuildComment(tab,
+
AT_PASS_OLD_CONSTR,
+ oldId,
+
ConstraintRelationId,
+ rel,
+ NIL,
+ con->conname,
+ NULL);
}
else
elog(ERROR, "unexpected statement
subtype: %d",
@@ -10607,18 +10688,22 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid
refRelId, char *cmd,
Constraint *con = castNode(Constraint,
stmt->def);
AlterTableCmd *cmd = makeNode(AlterTableCmd);
+ *drop_old = true;
+
cmd->subtype = AT_ReAddDomainConstraint;
cmd->def = (Node *) stmt;
tab->subcmds[AT_PASS_OLD_CONSTR] =
lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
/* recreate any comment on the constraint */
- RebuildConstraintComment(tab,
-
AT_PASS_OLD_CONSTR,
-
oldId,
-
NULL,
-
stmt->typeName,
-
con->conname);
+ RebuildComment(tab,
+ AT_PASS_OLD_CONSTR,
+ oldId,
+ ConstraintRelationId,
+ NULL,
+ stmt->typeName,
+ con->conname,
+ NULL);
}
else
elog(ERROR, "unexpected statement subtype: %d",
@@ -10634,25 +10719,27 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid
refRelId, char *cmd,
/*
* Subroutine for ATPostAlterTypeParse() to recreate any existing comment
- * for a table or domain constraint that is being rebuilt.
+ * for an index or a table or domain constraint that is being rebuilt.
*
* objid is the OID of the constraint.
- * Pass "rel" for a table constraint, or "domname" (domain's qualified name
- * as a string list) for a domain constraint.
+ * Pass "rel" for adding a comment on an index or a table constraint, or
+ * "domname" (domain's qualified name as a string list) for a domain
+ * constraint.
+ *
* (We could dig that info, as well as the conname, out of the pg_constraint
* entry; but callers already have them so might as well pass them.)
*/
static void
-RebuildConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
- Relation rel, List *domname,
- const char *conname)
+RebuildComment(AlteredTableInfo *tab, int pass, Oid objid, Oid classId,
+ Relation rel, List *domname,
+ const char *conname, const char *idxname)
{
CommentStmt *cmd;
char *comment_str;
AlterTableCmd *newcmd;
/* Look for comment for object wanted, and leave if none */
- comment_str = GetComment(objid, ConstraintRelationId, 0);
+ comment_str = GetComment(objid, classId, 0);
if (comment_str == NULL)
return;
@@ -10660,11 +10747,22 @@ RebuildConstraintComment(AlteredTableInfo *tab, int
pass, Oid objid,
cmd = makeNode(CommentStmt);
if (rel)
{
- cmd->objtype = OBJECT_TABCONSTRAINT;
- cmd->object = (Node *)
-
list_make3(makeString(get_namespace_name(RelationGetNamespace(rel))),
-
makeString(pstrdup(RelationGetRelationName(rel))),
- makeString(pstrdup(conname)));
+ if (conname)
+ {
+ cmd->objtype = OBJECT_TABCONSTRAINT;
+ cmd->object = (Node *)
+
list_make3(makeString(get_namespace_name(RelationGetNamespace(rel))),
+
makeString(pstrdup(RelationGetRelationName(rel))),
+
makeString(pstrdup(conname)));
+ }
+ /* else an index */
+ else
+ {
+ cmd->objtype = OBJECT_INDEX;
+ cmd->object = (Node *)
+
list_make2(makeString(get_namespace_name(RelationGetNamespace(rel))),
+
makeString(pstrdup(idxname)));
+ }
}
else
{
@@ -10683,25 +10781,6 @@ RebuildConstraintComment(AlteredTableInfo *tab, int
pass, Oid objid,
}
/*
- * Subroutine for ATPostAlterTypeParse(). Calls out to CheckIndexCompatible()
- * for the real analysis, then mutates the IndexStmt based on that verdict.
- */
-static void
-TryReuseIndex(Oid oldId, IndexStmt *stmt)
-{
- if (CheckIndexCompatible(oldId,
- stmt->accessMethod,
- stmt->indexParams,
- stmt->excludeOpNames))
- {
- Relation irel = index_open(oldId, NoLock);
-
- stmt->oldNode = irel->rd_node.relNode;
- index_close(irel, NoLock);
- }
-}
-
-/*
* Subroutine for ATPostAlterTypeParse().
*
* Stash the old P-F equality operator into the Constraint node, for possible
@@ -15527,7 +15606,7 @@ ATExecAttachPartitionIdx(List **wqueue, Relation
parentIdx, RangeVar *name)
partIdx->rd_opfamily,
parentIdx->rd_opfamily,
attmap,
-
RelationGetDescr(parentTbl)->natts))
+
RelationGetDescr(partTbl)->natts))
ereport(ERROR,
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
errmsg("cannot attach index \"%s\" as
a partition of index \"%s\"",
diff --git a/src/test/regress/expected/alter_table.out
b/src/test/regress/expected/alter_table.out
index b9aa4f189b..e97c68f981 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4021,3 +4021,56 @@ 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;
+-- test that ALTER TYPE correctly handles partitioned index rebuild correctly
+create table alttype_cleanup_idx (a int, b varchar(64), primary key (a, b))
partition by list (a);
+create table alttype_cleanup_idx1 partition of alttype_cleanup_idx for values
in (1);
+create table alttype_cleanup_idx2 partition of alttype_cleanup_idx for values
in (2);
+insert into alttype_cleanup_idx values (1, 'xxx');
+-- tables rewriten, indexes rebuilt
+alter table alttype_cleanup_idx alter b type varchar(128);
+-- primary key still works
+insert into alttype_cleanup_idx values (1, 'xxx');
+ERROR: duplicate key value violates unique constraint
"alttype_cleanup_idx1_pkey"
+DETAIL: Key (a, b)=(1, xxx) already exists.
+-- no rewrite, no indexes rebuilt
+alter table alttype_cleanup_idx alter b type varchar(10);
+-- primary key still works
+insert into alttype_cleanup_idx values (1, 'xxx');
+ERROR: duplicate key value violates unique constraint
"alttype_cleanup_idx1_pkey"
+DETAIL: Key (a, b)=(1, xxx) already exists.
+-- test that comments on child constraints are preserved through constraint
+-- reconstruction
+alter table alttype_cleanup_idx add c varchar(64);
+alter table alttype_cleanup_idx add constraint c_chk check (c <> '');
+comment on constraint c_chk on alttype_cleanup_idx is 'parent check';
+comment on constraint c_chk on alttype_cleanup_idx1 is 'child check 1';
+comment on constraint c_chk on alttype_cleanup_idx2 is 'child check 2';
+select conname, obj_description(oid, 'pg_constraint') from pg_constraint where
conname = 'c_chk' order by 2;
+ conname | obj_description
+---------+-----------------
+ c_chk | child check 1
+ c_chk | child check 2
+ c_chk | parent check
+(3 rows)
+
+-- tables rewritten, constraints rebuilt
+alter table alttype_cleanup_idx alter c type varchar(128);
+select conname, obj_description(oid, 'pg_constraint') from pg_constraint where
conname = 'c_chk' order by 2;
+ conname | obj_description
+---------+-----------------
+ c_chk | child check 1
+ c_chk | child check 2
+ c_chk | parent check
+(3 rows)
+
+-- no tables rewrite, constraints still rebuilt
+alter table alttype_cleanup_idx alter c type varchar(128);
+select conname, obj_description(oid, 'pg_constraint') from pg_constraint where
conname = 'c_chk' order by 2;
+ conname | obj_description
+---------+-----------------
+ c_chk | child check 1
+ c_chk | child check 2
+ c_chk | parent check
+(3 rows)
+
+drop table alttype_cleanup_idx;
diff --git a/src/test/regress/sql/alter_table.sql
b/src/test/regress/sql/alter_table.sql
index d675579977..1e67c789b8 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2668,3 +2668,34 @@ 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;
+
+-- test that ALTER TYPE correctly handles partitioned index rebuild correctly
+create table alttype_cleanup_idx (a int, b varchar(64), primary key (a, b))
partition by list (a);
+create table alttype_cleanup_idx1 partition of alttype_cleanup_idx for values
in (1);
+create table alttype_cleanup_idx2 partition of alttype_cleanup_idx for values
in (2);
+insert into alttype_cleanup_idx values (1, 'xxx');
+-- tables rewriten, indexes rebuilt
+alter table alttype_cleanup_idx alter b type varchar(128);
+-- primary key still works
+insert into alttype_cleanup_idx values (1, 'xxx');
+-- no rewrite, no indexes rebuilt
+alter table alttype_cleanup_idx alter b type varchar(10);
+-- primary key still works
+insert into alttype_cleanup_idx values (1, 'xxx');
+
+-- test that comments on child constraints are preserved through constraint
+-- reconstruction
+alter table alttype_cleanup_idx add c varchar(64);
+alter table alttype_cleanup_idx add constraint c_chk check (c <> '');
+comment on constraint c_chk on alttype_cleanup_idx is 'parent check';
+comment on constraint c_chk on alttype_cleanup_idx1 is 'child check 1';
+comment on constraint c_chk on alttype_cleanup_idx2 is 'child check 2';
+select conname, obj_description(oid, 'pg_constraint') from pg_constraint where
conname = 'c_chk' order by 2;
+-- tables rewritten, constraints rebuilt
+alter table alttype_cleanup_idx alter c type varchar(128);
+select conname, obj_description(oid, 'pg_constraint') from pg_constraint where
conname = 'c_chk' order by 2;
+-- no tables rewrite, constraints still rebuilt
+alter table alttype_cleanup_idx alter c type varchar(128);
+select conname, obj_description(oid, 'pg_constraint') from pg_constraint where
conname = 'c_chk' order by 2;
+
+drop table alttype_cleanup_idx;
