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;