From 2f1315caf1e08419b2fff56f6b63af3ebbb6e34c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Rodr=C3=ADguez?= Date: Thu, 14 May 2026 15:51:26 +0200 Subject: [PATCH v2 2/3] Skip index rewriting when possible on ALTER TABLE ALTER COLUMN TYPE for partitioned tables During ALTER TABLE ALTER COLUMN TYPE for a partitioned table with an index, the old deletion behavior: 1. Processes the parent table in ATRewriteCatalogs before processing any of its child tables 2. Runs ATPostAlterTypeCleanup for the table, which drops the partitioned index at the end. That drop cascades to the child indexes. 3. Processes the child tables in ATRewriteCatalogs. However, the child indexes were already dropped, so we never remember them in RememberAllDependentForRebuilding. 4. When indexes are eventually recreated, they always need to be reindexed. This change batches the deletions in ATRewriteCatalogs so that they happen at the end of a processing step and not in each call to ATPostAlterTypeCleanup (which happens for each processed table). This ensures that child indexes are tracked when ATExecAlterColumnType calls RememberAllDependentForRebuilding for each child table. After the change, child indexes are now recreated twice: once when the parent index is created, and once by themselves. We need for the child indexes to be created first. That way, they use the remembered information to decide whether a reindex is needed. When the parent index is created, it will automatically use existing child indexes. To enforce this behavior, we add a new AT_PASS_OLD_PARTITIONED_INDEX that recreates the parent indexes. --- src/backend/commands/tablecmds.c | 79 +++++++++++++++-------- src/test/regress/expected/alter_table.out | 7 +- src/test/regress/sql/alter_table.sql | 3 +- 3 files changed, 55 insertions(+), 34 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 38f9ffcd04f..c85463c4175 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -157,6 +157,7 @@ typedef enum AlterTablePass AT_PASS_ADD_COL, /* ADD COLUMN */ AT_PASS_SET_EXPRESSION, /* ALTER SET EXPRESSION */ AT_PASS_OLD_INDEX, /* re-add existing indexes */ + AT_PASS_OLD_PARTITIONED_INDEX, /* re-add existing partitioned indexes */ AT_PASS_OLD_CONSTR, /* re-add existing constraints */ /* We could support a RENAME COLUMN pass here, but not currently used */ AT_PASS_ADD_CONSTR, /* ADD constraints (initial examination) */ @@ -689,8 +690,7 @@ static void RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableT static void RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab); static void RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab); static void RememberStatisticsForRebuilding(Oid stxoid, AlteredTableInfo *tab); -static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, - LOCKMODE lockmode); +static void ATPostAlterTypeCleanup(List **wqueue, ObjectAddresses *objects, AlteredTableInfo *tab, LOCKMODE lockmode); static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd, List **wqueue, LOCKMODE lockmode, bool rewrite); @@ -5381,6 +5381,8 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode, */ for (AlterTablePass pass = 0; pass < AT_NUM_PASSES; pass++) { + ObjectAddresses *todelete = new_object_addresses(); + /* Go through each table that needs to be processed */ foreach(ltab, *wqueue) { @@ -5406,10 +5408,14 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode, /* * After the ALTER TYPE or SET EXPRESSION pass, do cleanup work * (this is not done in ATExecAlterColumnType since it should be - * done only once if multiple columns of a table are altered). + * done only once if multiple columns of a table are altered). The + * objects pending deletion will be batched in the todelete list, + * to be deleted at the end of the pass. Otherwise, we will drop + * dependent objects before they are properly procesed. This is a + * problem when we try to avoid rewrites for child indexes. */ if (pass == AT_PASS_ALTER_TYPE || pass == AT_PASS_SET_EXPRESSION) - ATPostAlterTypeCleanup(wqueue, tab, lockmode); + ATPostAlterTypeCleanup(wqueue, todelete, tab, lockmode); if (tab->rel) { @@ -5417,6 +5423,20 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode, tab->rel = NULL; } } + + /* + * After the full pass, we drop all the objects that we accumulated, + * then free the address list. It should be okay to use DROP_RESTRICT + * here, since nothing else should be depending on these objects. + */ + performMultipleDeletions(todelete, DROP_RESTRICT, PERFORM_DELETION_INTERNAL); + + free_object_addresses(todelete); + + /* + * The objects will get recreated during subsequent passes over the + * work queue. + */ } /* Check to see if a toast table must be added. */ @@ -15684,25 +15704,18 @@ RememberStatisticsForRebuilding(Oid stxoid, AlteredTableInfo *tab) /* * Cleanup after we've finished all the ALTER TYPE or SET EXPRESSION * operations for a particular relation. We have to drop and recreate all the - * indexes and constraints that depend on the altered columns. We do the - * actual dropping here, but re-creation is managed by adding work queue - * entries to do those steps later. + * indexes and constraints that depend on the altered columns. We record the + * objects that need to be deleted here, so they can be dropped by the caller + * in one go after this method has been called for each table. The re-creation, + * however, is managed by adding work queue entries to do those steps later. */ static void -ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode) +ATPostAlterTypeCleanup(List **wqueue, ObjectAddresses *objects, AlteredTableInfo *tab, LOCKMODE lockmode) { ObjectAddress obj; - ObjectAddresses *objects; ListCell *def_item; ListCell *oid_item; - /* - * Collect all the constraints and indexes to drop so we can process them - * in a single call. That way we don't have to worry about dependencies - * among them. - */ - objects = new_object_addresses(); - /* * Re-parse the index and constraint definitions, and attach them to the * appropriate work queue entries. We do this before dropping because in @@ -15859,16 +15872,10 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode) } /* - * It should be okay to use DROP_RESTRICT here, since nothing else should - * be depending on these objects. - */ - performMultipleDeletions(objects, DROP_RESTRICT, PERFORM_DELETION_INTERNAL); - - free_object_addresses(objects); - - /* - * The objects will get recreated during subsequent passes over the work - * queue. + * The caller will take care of deleting the objects. We need to batch + * the deletions together instead of dropping here. Otherwise, child + * indexes will be dropped when dropping their parent partitioned index, + * which means they will never be tracked for rebuilding. */ } @@ -15952,6 +15959,7 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd, { IndexStmt *stmt = (IndexStmt *) stm; AlterTableCmd *newcmd; + char relkind = get_rel_relkind(oldId); if (!rewrite) TryReuseIndex(oldId, stmt); @@ -15962,8 +15970,23 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd, 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); + + /* + * We process partitioned indexes at a later stage. This ensures + * that we won't create duplicate indexes. We rely on postgres + * automatically attaching existing child indexes to a new + * partitioned index. + * + * We could choose to skip child indexes and let them be recreated + * by the parent, but that would force a reindex that we can + * potentially avoid. + */ + if (relkind == RELKIND_PARTITIONED_INDEX) + tab->subcmds[AT_PASS_OLD_PARTITIONED_INDEX] = + lappend(tab->subcmds[AT_PASS_OLD_PARTITIONED_INDEX], newcmd); + else + tab->subcmds[AT_PASS_OLD_INDEX] = + lappend(tab->subcmds[AT_PASS_OLD_INDEX], newcmd); } else if (IsA(stm, AlterTableStmt)) { diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 1efdadf5e57..47b08b3d7b7 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -2327,8 +2327,7 @@ select conname, obj_description(oid, 'pg_constraint') as desc -- Don't remove this DROP, it exposes bug #15672 drop table at_partitioned; -- Alter column type when no index rewrite is required --- for partitioned tables. Reindex is required although --- it shouldn't be. +-- for partitioned tables. create table at_idx_part (id int, code varchar(10), primary key (id, code)) partition by range (id); @@ -2358,10 +2357,10 @@ select relname, at_idx_part | t | none at_idx_part_code_idx | f | none at_idx_part_p1 | t | own - at_idx_part_p1_code_idx | f | own + at_idx_part_p1_code_idx | f | orig at_idx_part_p1_pkey | f | own at_idx_part_p2 | t | own - at_idx_part_p2_code_idx | f | own + at_idx_part_p2_code_idx | f | orig at_idx_part_p2_pkey | f | own at_idx_part_pkey | f | none (9 rows) diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 54dc9a85b8b..c78598308e4 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -1530,8 +1530,7 @@ select conname, obj_description(oid, 'pg_constraint') as desc drop table at_partitioned; -- Alter column type when no index rewrite is required --- for partitioned tables. Reindex is required although --- it shouldn't be. +-- for partitioned tables. create table at_idx_part (id int, code varchar(10), primary key (id, code)) partition by range (id); -- 2.52.0