From a13ea4c768ec9233d0eede3e197d165c58957f28 Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Fri, 21 Aug 2026 13:01:21 -0400 Subject: [PATCH v14 2/2] Preserve leaf partition index properties across ALTER COLUMN TYPE When ALTER TABLE ... ALTER COLUMN TYPE (or SET EXPRESSION) rebuilds a partitioned index, it drops each old leaf partition index and recreates it by cloning the parent index's structure. The old leaf index's catalog rows are deleted, and the clone reproduces only the DDL-expressible structure, so the leaf index lost its non-DDL properties: custom name, comment, replica identity, cluster-on marking, and any reloptions set independently of the parent (e.g. ALTER INDEX ... SET (fillfactor=...)). A leaf index created directly on the partition or a plain table's index kept these via the existing top-level remember/restore path; only indexes descended from a partitioned index via CREATE INDEX ON parent or ATTACH were affected. Capture each old leaf index's non-DDL properties before the drop, transfer the matching entry onto each child IndexStmt during the partition recursion, and re-apply them after creating the new index and its catalog tuples. --- src/backend/commands/indexcmds.c | 106 ++++++++++++++++++++++ src/backend/commands/tablecmds.c | 88 ++++++++++++++++++ src/include/nodes/parsenodes.h | 33 +++++++ src/test/regress/expected/alter_table.out | 75 ++++++++++++++- src/test/regress/sql/alter_table.sql | 71 +++++++++++++++ src/tools/pgindent/typedefs.list | 1 + 6 files changed, 370 insertions(+), 4 deletions(-) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 4e592ea7785..79025f61d12 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -106,6 +106,8 @@ static char *ChooseIndexName(const char *tabname, Oid namespaceId, const List *colnames, const List *exclusionOpNames, bool primary, bool isconstraint); static char *ChooseIndexNameAddition(const List *colnames); +static void TransferPartitionIndexProps(const IndexStmt *stmt, Oid childRelid, + IndexStmt *childStmt); static List *ChooseIndexColumnNames(Relation rel, const List *indexElems); static char *ChooseIndexExpressionName(Relation rel, Node *indexExpr); static bool ChooseIndexExpressionName_walker(Node *node, @@ -551,6 +553,46 @@ SetIndexStatTargets(Oid indexRelationId, List *stattargets) } +/* + * Copy this partition's non-DDL properties (name, comment, replica identity, + * cluster-on, per-column stats targets) from the list of all leaf partitions' + * properties into the correct scalar fields in the newly created IndexStmt + * for the child partition index as part of an ALTER COLUMN TYPE (or SET + * EXPRESSION) operation. + * + * These were saved in a list before dropping the index in an earlier phase. + * After we create the child partition index, we'll update the catalog table + * entries according to these values. + */ +static void +TransferPartitionIndexProps(const IndexStmt *stmt, Oid childRelid, + IndexStmt *childStmt) +{ + foreach_node(PartitionIndexProps, props, stmt->oldPartIndexProps) + { + /* + * Match entries by the partition table's OID (stable), since the old + * index's OID is gone. + */ + if (props->partrelid != childRelid) + continue; + + childStmt->idxname = pstrdup(props->idxname); + childStmt->idxcomment = props->idxcomment; + childStmt->idxisreplident = props->isreplident; + childStmt->idxisclustered = props->isclustered; + childStmt->idxstattargets = props->stattargets; + + /* + * Override the parent's reloptions with the leaf's own, so + * independently-set leaf options survive. + */ + childStmt->options = props->reloptions; + return; + } +} + + /* * DefineIndex * Creates a new index. @@ -1372,6 +1414,58 @@ DefineIndex(ParseState *pstate, if (stmt->idxstattargets != NIL) SetIndexStatTargets(indexRelationId, stmt->idxstattargets); + /* + * We set indisclustered/indisreplident with a direct single-row pg_index + * update, not mark_index_clustered()/relation_mark_replica_identity() + * because those clear the flag on sibling indexes which are mid-drop + * here. That's safe to skip because at most one index per table carries + * each flag and it is this newly-created one. + */ + if (stmt->idxisclustered || stmt->idxisreplident) + { + Relation pg_index; + HeapTuple idxtuple; + Form_pg_index indexForm; + + pg_index = table_open(IndexRelationId, RowExclusiveLock); + idxtuple = SearchSysCacheCopy1(INDEXRELID, + ObjectIdGetDatum(indexRelationId)); + if (!HeapTupleIsValid(idxtuple)) + elog(ERROR, "cache lookup failed for index %u", indexRelationId); + indexForm = (Form_pg_index) GETSTRUCT(idxtuple); + + if (stmt->idxisclustered) + indexForm->indisclustered = true; + if (stmt->idxisreplident) + indexForm->indisreplident = true; + + CatalogTupleUpdate(pg_index, &idxtuple->t_self, idxtuple); + heap_freetuple(idxtuple); + table_close(pg_index, RowExclusiveLock); + } + + /* Replica identity also marks the owning table's relreplident. */ + if (stmt->idxisreplident) + { + Relation pg_class; + Oid heapId = IndexGetRelation(indexRelationId, false); + HeapTuple ctup; + Form_pg_class classForm; + + pg_class = table_open(RelationRelationId, RowExclusiveLock); + ctup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(heapId)); + if (!HeapTupleIsValid(ctup)) + elog(ERROR, "cache lookup failed for relation %u", heapId); + classForm = (Form_pg_class) GETSTRUCT(ctup); + if (classForm->relreplident != REPLICA_IDENTITY_INDEX) + { + classForm->relreplident = REPLICA_IDENTITY_INDEX; + CatalogTupleUpdate(pg_class, &ctup->t_self, ctup); + } + heap_freetuple(ctup); + table_close(pg_class, RowExclusiveLock); + } + if (partitioned) { PartitionDesc partdesc; @@ -1594,6 +1688,18 @@ DefineIndex(ParseState *pstate, attmap, NULL); + /* + * generateClonedIndexStmt() only clones DDL-expressible + * properties, so transfer the old leaf index's non-DDL + * properties into the child IndexStmt. The child also + * needs a pointer to the list in case it is an + * intermediate partitioned index and needs its own + * children to be able to find their entries upon + * recursing. + */ + childStmt->oldPartIndexProps = stmt->oldPartIndexProps; + TransferPartitionIndexProps(stmt, childRelid, childStmt); + /* * Recurse as the starting user ID. Callee will use that * for permission checks, then switch again. diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index f9b1b0aa82d..e5fe941e632 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -707,6 +707,7 @@ static void RebuildConstraintComment(AlteredTableInfo *tab, AlterTablePass pass, Oid objid, Relation rel, List *domname, const char *conname); static void TryReuseIndex(Oid oldId, IndexStmt *stmt); +static void RememberPartitionIndexProps(Oid indoid, IndexStmt *stmt); static List *GetIndexStatTargets(Oid indexOid); static void TryReuseForeignKey(Oid oldId, Constraint *con); static ObjectAddress ATExecAlterColumnGenericOptions(Relation rel, const char *colName, @@ -15981,6 +15982,13 @@ RememberWholeRowDependentForRebuilding(AlteredTableInfo *tab, AlterTableType sub /* * Subroutine for ATExecAlterColumnType: remember that a replica identity * needs to be reset. + * + * We save the index by name and restore it later via an AT_ReplicaIdentity + * subcommand (see ATPostAlterTypeCleanup), rather than stamping + * indisreplident at creation like the partition-leaf path. The top-level + * index may not be rebuilt at all, or may have live siblings whose flag must + * be cleared, so it needs the relation-wide relation_mark_replica_identity() + * run after the rebuild. */ static void RememberReplicaIdentityForRebuilding(Oid indoid, AlteredTableInfo *tab) @@ -16435,6 +16443,8 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, Oid ownerId, IndexStmt *stmt = (IndexStmt *) stm; AlterTableCmd *newcmd; + /* capture leaf indexes' non-DDL properties before they're dropped */ + RememberPartitionIndexProps(oldId, stmt); if (!rewrite) TryReuseIndex(oldId, stmt); stmt->reset_default_tblspc = true; @@ -16466,6 +16476,7 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, Oid ownerId, indstmt = castNode(IndexStmt, cmd->def); indoid = get_constraint_index(oldId); + RememberPartitionIndexProps(indoid, indstmt); if (!rewrite) TryReuseIndex(indoid, indstmt); /* keep any comment on the index */ @@ -16620,6 +16631,83 @@ RebuildConstraintComment(AlteredTableInfo *tab, AlterTablePass pass, Oid objid, tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd); } +/* + * Before a partitioned index is dropped and rebuilt as part of an ALTER + * COLUMN TYPE (or SET EXPRESSION), capture each of its leaf partition indexes' + * non-DDL properties (name, comment, replica identity, cluster-on, per-column + * stat targets) into a list saved on the parent index's IndexStmt. These are + * not reproduced by the CREATE INDEX round-trip and would otherwise be lost + * when recreating the leaf index. + * + * Must run before the drop, while the old leaf indexes still exist. + */ +static void +RememberPartitionIndexProps(Oid indoid, IndexStmt *stmt) +{ + List *indexOids; + + if (get_rel_relkind(indoid) != RELKIND_PARTITIONED_INDEX) + return; + + indexOids = find_all_inheritors(indoid, NoLock, NULL); + foreach_oid(leafIndexOid, indexOids) + { + PartitionIndexProps *props; + HeapTuple idxtup; + HeapTuple classtup; + Form_pg_index idxform; + Form_pg_class classform; + Datum reloptions; + bool rel_isnull; + + if (leafIndexOid == indoid) + continue; + + idxtup = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(leafIndexOid)); + if (!HeapTupleIsValid(idxtup)) + continue; + idxform = (Form_pg_index) GETSTRUCT(idxtup); + + classtup = SearchSysCache1(RELOID, ObjectIdGetDatum(leafIndexOid)); + if (!HeapTupleIsValid(classtup)) + { + ReleaseSysCache(idxtup); + continue; + } + classform = (Form_pg_class) GETSTRUCT(classtup); + + /* only direct/indirect leaf (storage) indexes carry these props */ + if (classform->relkind == RELKIND_PARTITIONED_INDEX) + { + ReleaseSysCache(classtup); + ReleaseSysCache(idxtup); + continue; + } + + props = makeNode(PartitionIndexProps); + props->partrelid = idxform->indrelid; + props->idxname = pstrdup(NameStr(classform->relname)); + props->idxcomment = GetComment(leafIndexOid, RelationRelationId, 0); + props->isreplident = idxform->indisreplident; + props->isclustered = idxform->indisclustered; + props->stattargets = NIL; + props->reloptions = NIL; + + reloptions = SysCacheGetAttr(RELOID, classtup, + Anum_pg_class_reloptions, &rel_isnull); + if (!rel_isnull) + props->reloptions = untransformRelOptions(reloptions); + + ReleaseSysCache(classtup); + ReleaseSysCache(idxtup); + + props->stattargets = GetIndexStatTargets(leafIndexOid); + + stmt->oldPartIndexProps = lappend(stmt->oldPartIndexProps, props); + } + list_free(indexOids); +} + /* * Collect the per-column statistics targets of an index into a list of * IndexStatTarget nodes. Returns NIL if none are set. diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 84f7f0e7d3b..eddafc9920e 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -3664,8 +3664,21 @@ typedef struct IndexStmt * index to the new and must be explicitly saved before dropping the old * index and restored after creating the new index. */ + bool idxisreplident; /* restore this index as REPLICA IDENTITY */ + bool idxisclustered; /* restore CLUSTER ON this index */ char *idxcomment; /* comment to apply to index, or NULL */ List *idxstattargets; /* list of IndexStatTarget to restore */ + + /* + * For a partitioned index, oldPartIndexProps holds one entry per old leaf + * index (across all partition levels). The whole list is propagated to + * every IndexStmt in DefineIndex()'s recursion so that intermediate + * levels can still find deeper leaves' entries. Each stmt then copies its + * own matching entry into the corresponding scalar fields in the + * IndexStmt (see TransferPartitionIndexProps). + */ + List *oldPartIndexProps; /* list of PartitionIndexProps + * (partitioned index rebuild only) */ } IndexStmt; /* one per-column statistics target carried across an index rebuild */ @@ -3678,6 +3691,26 @@ typedef struct IndexStatTarget int stattarget; /* attstattarget value to restore */ } IndexStatTarget; +/* + * Non-DDL properties of one old leaf partition index, captured before it is + * dropped during ALTER COLUMN TYPE or SET EXPRESSION so they can be re-applied + * to the rebuilt child index. + */ +typedef struct PartitionIndexProps +{ + pg_node_attr(no_equal, no_query_jumble) + + NodeTag type; + Oid partrelid; /* partition table owning this index */ + char *idxname; /* index name to restore */ + char *idxcomment; /* comment, or NULL */ + bool isreplident; /* was replica identity */ + bool isclustered; /* was clustered on */ + List *stattargets; /* list of IndexStatTarget */ + List *reloptions; /* index's own reloptions (untransformed + * DefElem list), or NIL */ +} PartitionIndexProps; + /* ---------------------- * Create Statistics Statement * ---------------------- diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 8b6d85461f9..bcca6499d63 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -2304,13 +2304,13 @@ select relname, from pg_class c left join old_oids using (relname) where relname like 'at_partitioned%' order by relname; - relname | orig_oid | storage | desc -------------------------------+----------+---------+-------------- + relname | orig_oid | storage | desc +------------------------------+----------+---------+--------------- at_partitioned | t | none | at_partitioned_0 | t | own | - at_partitioned_0_id_name_key | f | own | + at_partitioned_0_id_name_key | f | own | child 0 index at_partitioned_1 | t | own | - at_partitioned_1_id_name_key | f | own | + at_partitioned_1_id_name_key | f | own | child 1 index at_partitioned_id_name_key | f | none | parent index (6 rows) @@ -2346,6 +2346,73 @@ select c.relname, a.attnum, a.attstattarget (3 rows) drop table at_reb_plain; +-- Partitioned tables' leaf partitions should all preserve their properties +-- across an ALTER COLUMN TYPE-triggered rebuild. +create table at_reb (id int not null, val int not null) partition by range (id); +create table at_reb_1 partition of at_reb for values from (0) to (100); +create index at_reb_expr on at_reb ((val + 1)) with (fillfactor = 71); +create unique index at_reb_uniq on at_reb (id, val); +-- give the leaf indexes stable, custom names (also exercises name preservation) +alter index at_reb_1_val_1_idx rename to at_reb_expr_leaf; +alter index at_reb_1_id_val_idx rename to at_reb_uniq_leaf; +-- load the leaf indexes with every non-DDL property +comment on index at_reb_expr_leaf is 'leaf expr index comment'; +alter index at_reb_expr_leaf alter column 1 set statistics 543; +alter index at_reb_expr_leaf set (fillfactor = 55); +alter table at_reb_1 replica identity using index at_reb_uniq_leaf; +alter table at_reb_1 cluster on at_reb_expr_leaf; +-- Snapshot each catalog row describing the two leaf indexes as jsonb. Remove +-- the columns that legitimately differ across the rebuild: the physical +-- identity (oid, relfilenode, and their indexrelid/indrelid/attrelid echoes), +-- and the size estimates (relpages, reltuples, relallvisible, relallfrozen), +-- which are recomputed by the fresh index build rather than carried over. Any +-- other difference fails the test. +create function at_reb_snapshot() returns table(cat text, disc text, body jsonb) +language sql stable as $$ + select 'pg_class', c.relname, + to_jsonb(c) - '{oid,relfilenode,relpages,reltuples,relallvisible, + relallfrozen}'::text[] + from pg_class c + where c.relname in ('at_reb_expr_leaf', 'at_reb_uniq_leaf') + union all + select 'pg_index', c.relname, + to_jsonb(i) - '{indexrelid,indrelid}'::text[] + from pg_index i join pg_class c on c.oid = i.indexrelid + where c.relname in ('at_reb_expr_leaf', 'at_reb_uniq_leaf') + union all + select 'pg_attribute', c.relname || '.' || a.attnum, + to_jsonb(a) - '{attrelid}'::text[] + from pg_attribute a join pg_class c on c.oid = a.attrelid + where c.relname in ('at_reb_expr_leaf', 'at_reb_uniq_leaf') + and a.attnum > 0 + union all + select 'comment', c.relname, to_jsonb(obj_description(c.oid, 'pg_class')) + from pg_class c + where c.relname in ('at_reb_expr_leaf', 'at_reb_uniq_leaf') + union all + -- owning table row, for relreplident (replica identity is recorded here too) + select 'pg_class_owner', tc.relname, to_jsonb(tc.relreplident) + from pg_index i join pg_class c on c.oid = i.indexrelid + join pg_class tc on tc.oid = i.indrelid + where c.relname in ('at_reb_expr_leaf', 'at_reb_uniq_leaf'); +$$; +create temp table at_reb_before as select * from at_reb_snapshot(); +-- No-op type change: same type, but still rebuilds the leaf indexes. +alter table at_reb alter column val type int; +create temp table at_reb_after as select * from at_reb_snapshot(); +-- Both directions must be empty: nothing lost, nothing unexpectedly changed. +select 'lost/changed' as dir, cat, disc from ( + select * from at_reb_before except select * from at_reb_after) d +union all +select 'appeared/changed', cat, disc from ( + select * from at_reb_after except select * from at_reb_before) d +order by 1, 2, 3; + dir | cat | disc +-----+-----+------ +(0 rows) + +drop function at_reb_snapshot(); +drop table at_reb; -- disallow recursive containment of row types create temp table recur1 (f1 int); alter table recur1 add column f2 recur1; -- fails diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 8bdfc4262ef..017e20a4436 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -1545,6 +1545,77 @@ select c.relname, a.attnum, a.attstattarget order by c.relname, a.attnum; drop table at_reb_plain; +-- Partitioned tables' leaf partitions should all preserve their properties +-- across an ALTER COLUMN TYPE-triggered rebuild. +create table at_reb (id int not null, val int not null) partition by range (id); +create table at_reb_1 partition of at_reb for values from (0) to (100); +create index at_reb_expr on at_reb ((val + 1)) with (fillfactor = 71); +create unique index at_reb_uniq on at_reb (id, val); +-- give the leaf indexes stable, custom names (also exercises name preservation) +alter index at_reb_1_val_1_idx rename to at_reb_expr_leaf; +alter index at_reb_1_id_val_idx rename to at_reb_uniq_leaf; + +-- load the leaf indexes with every non-DDL property +comment on index at_reb_expr_leaf is 'leaf expr index comment'; +alter index at_reb_expr_leaf alter column 1 set statistics 543; +alter index at_reb_expr_leaf set (fillfactor = 55); +alter table at_reb_1 replica identity using index at_reb_uniq_leaf; +alter table at_reb_1 cluster on at_reb_expr_leaf; + +-- Snapshot each catalog row describing the two leaf indexes as jsonb. Remove +-- the columns that legitimately differ across the rebuild: the physical +-- identity (oid, relfilenode, and their indexrelid/indrelid/attrelid echoes), +-- and the size estimates (relpages, reltuples, relallvisible, relallfrozen), +-- which are recomputed by the fresh index build rather than carried over. Any +-- other difference fails the test. +create function at_reb_snapshot() returns table(cat text, disc text, body jsonb) +language sql stable as $$ + select 'pg_class', c.relname, + to_jsonb(c) - '{oid,relfilenode,relpages,reltuples,relallvisible, + relallfrozen}'::text[] + from pg_class c + where c.relname in ('at_reb_expr_leaf', 'at_reb_uniq_leaf') + union all + select 'pg_index', c.relname, + to_jsonb(i) - '{indexrelid,indrelid}'::text[] + from pg_index i join pg_class c on c.oid = i.indexrelid + where c.relname in ('at_reb_expr_leaf', 'at_reb_uniq_leaf') + union all + select 'pg_attribute', c.relname || '.' || a.attnum, + to_jsonb(a) - '{attrelid}'::text[] + from pg_attribute a join pg_class c on c.oid = a.attrelid + where c.relname in ('at_reb_expr_leaf', 'at_reb_uniq_leaf') + and a.attnum > 0 + union all + select 'comment', c.relname, to_jsonb(obj_description(c.oid, 'pg_class')) + from pg_class c + where c.relname in ('at_reb_expr_leaf', 'at_reb_uniq_leaf') + union all + -- owning table row, for relreplident (replica identity is recorded here too) + select 'pg_class_owner', tc.relname, to_jsonb(tc.relreplident) + from pg_index i join pg_class c on c.oid = i.indexrelid + join pg_class tc on tc.oid = i.indrelid + where c.relname in ('at_reb_expr_leaf', 'at_reb_uniq_leaf'); +$$; + +create temp table at_reb_before as select * from at_reb_snapshot(); + +-- No-op type change: same type, but still rebuilds the leaf indexes. +alter table at_reb alter column val type int; + +create temp table at_reb_after as select * from at_reb_snapshot(); + +-- Both directions must be empty: nothing lost, nothing unexpectedly changed. +select 'lost/changed' as dir, cat, disc from ( + select * from at_reb_before except select * from at_reb_after) d +union all +select 'appeared/changed', cat, disc from ( + select * from at_reb_after except select * from at_reb_before) d +order by 1, 2, 3; + +drop function at_reb_snapshot(); +drop table at_reb; + -- disallow recursive containment of row types create temp table recur1 (f1 int); alter table recur1 add column f2 recur1; -- fails diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 6a464f80de1..14a5b66f579 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -2218,6 +2218,7 @@ PartitionDispatch PartitionElem PartitionHashBound PartitionIndexExtDepEntry +PartitionIndexProps PartitionKey PartitionListValue PartitionMap -- 2.47.3