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.
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..3237f15125 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 'alttype_cleanup_idx
check constraint';
+comment on constraint c_chk on alttype_cleanup_idx1 is 'alttype_cleanup_idx1
check constraint';
+comment on constraint c_chk on alttype_cleanup_idx2 is 'alttype_cleanup_idx2
check constraint';
+select conname, obj_description(oid, 'pg_constraint') from pg_constraint where
conname = 'c_chk' order by 1, 2;
+ conname | obj_description
+---------+---------------------------------------
+ c_chk | alttype_cleanup_idx1 check constraint
+ c_chk | alttype_cleanup_idx2 check constraint
+ c_chk | alttype_cleanup_idx check constraint
+(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 1, 2;
+ conname | obj_description
+---------+---------------------------------------
+ c_chk | alttype_cleanup_idx1 check constraint
+ c_chk | alttype_cleanup_idx2 check constraint
+ c_chk | alttype_cleanup_idx check constraint
+(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 1, 2;
+ conname | obj_description
+---------+---------------------------------------
+ c_chk | alttype_cleanup_idx1 check constraint
+ c_chk | alttype_cleanup_idx2 check constraint
+ c_chk | alttype_cleanup_idx check constraint
+(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..9c1aafc94e 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 'alttype_cleanup_idx
check constraint';
+comment on constraint c_chk on alttype_cleanup_idx1 is 'alttype_cleanup_idx1
check constraint';
+comment on constraint c_chk on alttype_cleanup_idx2 is 'alttype_cleanup_idx2
check constraint';
+select conname, obj_description(oid, 'pg_constraint') from pg_constraint where
conname = 'c_chk' order by 1, 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 1, 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 1, 2;
+
+drop table alttype_cleanup_idx;
