From: Michael Paquier Date: Sun, 13 Oct 2019 08:51:55 +0000 (+0900) Subject: Fix dependency handling of column drop with partitioned tables X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=1df5875d39383b3981b804666ee1f4b0ff65943f;p=postgresql Fix dependency handling of column drop with partitioned tables When dropping a column on a partitioned table which has one or more partitioned indexes, the operation was failing as dependencies with partitioned indexes using the column dropped were not getting removed in a way consistent with the columns involved across all the relations part of an inheritance tree. This commit refactors the code executing column drop so as all the columns from an inheritance tree to remove are gathered first, and dropped all at the end. This way, we let the dependency machinery sort out by itself the deletion of all the columns with the partitioned indexes across a partition tree. This issue has been introduced by 1d92a0c, so backpatch down to REL_12_STABLE. Author: Amit Langote, Michael Paquier Reviewed-by: Álvaro Herrera, Ashutosh Sharma Discussion: https://postgr.es/m/CA+HiwqE9kuBsZ3b5pob2-cvE8ofzPWs-og+g8bKKGnu6b4-yTQ@mail.gmail.com Backpatch-through: 12 --- diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index ba8f4459f3..8d25d14772 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -401,7 +401,8 @@ static void ATPrepDropColumn(List **wqueue, Relation rel, bool recurse, bool rec static ObjectAddress ATExecDropColumn(List **wqueue, Relation rel, const char *colName, DropBehavior behavior, bool recurse, bool recursing, - bool missing_ok, LOCKMODE lockmode); + bool missing_ok, LOCKMODE lockmode, + ObjectAddresses *addrs); static ObjectAddress ATExecAddIndex(AlteredTableInfo *tab, Relation rel, IndexStmt *stmt, bool is_rebuild, LOCKMODE lockmode); static ObjectAddress ATExecAddConstraint(List **wqueue, @@ -4273,12 +4274,14 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel, case AT_DropColumn: /* DROP COLUMN */ address = ATExecDropColumn(wqueue, rel, cmd->name, cmd->behavior, false, false, - cmd->missing_ok, lockmode); + cmd->missing_ok, lockmode, + NULL); break; case AT_DropColumnRecurse: /* DROP COLUMN with recursion */ address = ATExecDropColumn(wqueue, rel, cmd->name, cmd->behavior, true, false, - cmd->missing_ok, lockmode); + cmd->missing_ok, lockmode, + NULL); break; case AT_AddIndex: /* ADD INDEX */ address = ATExecAddIndex(tab, rel, (IndexStmt *) cmd->def, false, @@ -7013,13 +7016,22 @@ ATPrepDropColumn(List **wqueue, Relation rel, bool recurse, bool recursing, } /* - * Return value is the address of the dropped column. + * Drops column 'colName' from relation 'rel' and returns the address of the + * dropped column. The column is also dropped (or marked as no longer + * inherited from relation) from the relation's inheritance children, if any. + * + * In the recursive invocations for inheritance child relations, instead of + * dropping the column directly (if to be dropped at all), its object address + * is added to 'addrs', which must be non-NULL in such invocations. All + * columns are dropped at the same time after all the children have been + * checked recursively. */ static ObjectAddress ATExecDropColumn(List **wqueue, Relation rel, const char *colName, DropBehavior behavior, bool recurse, bool recursing, - bool missing_ok, LOCKMODE lockmode) + bool missing_ok, LOCKMODE lockmode, + ObjectAddresses *addrs) { HeapTuple tuple; Form_pg_attribute targetatt; @@ -7032,6 +7044,11 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName, if (recursing) ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE); + /* Initialize addrs on the first invocation */ + Assert(!recursing || addrs != NULL); + if (!recursing) + addrs = new_object_addresses(); + /* * get the number of the attribute */ @@ -7144,7 +7161,7 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName, /* Time to delete this child column, too */ ATExecDropColumn(wqueue, childrel, colName, behavior, true, true, - false, lockmode); + false, lockmode, addrs); } else { @@ -7180,14 +7197,18 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName, table_close(attr_rel, RowExclusiveLock); } - /* - * Perform the actual column deletion - */ + /* Add object to delete */ object.classId = RelationRelationId; object.objectId = RelationGetRelid(rel); object.objectSubId = attnum; + add_exact_object_address(&object, addrs); - performDeletion(&object, behavior, 0); + if (!recursing) + { + /* Recursion has ended, drop everything that was collected */ + performMultipleDeletions(addrs, behavior, 0); + free_object_addresses(addrs); + } return object; } diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out index c143df5114..ec1d4eaef4 100644 --- a/src/test/regress/expected/indexing.out +++ b/src/test/regress/expected/indexing.out @@ -1258,3 +1258,64 @@ ERROR: cannot drop inherited constraint "parted_uniq_detach_test1_a_key" of rel alter table parted_uniq_detach_test detach partition parted_uniq_detach_test1; alter table parted_uniq_detach_test1 drop constraint parted_uniq_detach_test1_a_key; drop table parted_uniq_detach_test, parted_uniq_detach_test1; +-- check that dropping a column takes with it any partitioned indexes +-- depending on it. +create table parted_index_col_drop(a int, b int, c int) + partition by list (a); +create table parted_index_col_drop1 partition of parted_index_col_drop + for values in (1) partition by list (a); +-- leave this partition without children. +create table parted_index_col_drop2 partition of parted_index_col_drop + for values in (2) partition by list (a); +create table parted_index_col_drop11 partition of parted_index_col_drop1 + for values in (1); +create index on parted_index_col_drop (b); +create index on parted_index_col_drop (c); +create index on parted_index_col_drop (b, c); +alter table parted_index_col_drop drop column c; +\d parted_index_col_drop + Partitioned table "public.parted_index_col_drop" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | +Partition key: LIST (a) +Indexes: + "parted_index_col_drop_b_idx" btree (b) +Number of partitions: 2 (Use \d+ to list them.) + +\d parted_index_col_drop1 + Partitioned table "public.parted_index_col_drop1" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | +Partition of: parted_index_col_drop FOR VALUES IN (1) +Partition key: LIST (a) +Indexes: + "parted_index_col_drop1_b_idx" btree (b) +Number of partitions: 1 (Use \d+ to list them.) + +\d parted_index_col_drop2 + Partitioned table "public.parted_index_col_drop2" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | +Partition of: parted_index_col_drop FOR VALUES IN (2) +Partition key: LIST (a) +Indexes: + "parted_index_col_drop2_b_idx" btree (b) +Number of partitions: 0 + +\d parted_index_col_drop11 + Table "public.parted_index_col_drop11" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | +Partition of: parted_index_col_drop1 FOR VALUES IN (1) +Indexes: + "parted_index_col_drop11_b_idx" btree (b) + +drop table parted_index_col_drop; diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql index cc3d0abfb7..f6a3767918 100644 --- a/src/test/regress/sql/indexing.sql +++ b/src/test/regress/sql/indexing.sql @@ -703,3 +703,24 @@ alter table parted_uniq_detach_test1 drop constraint parted_uniq_detach_test1_a_ alter table parted_uniq_detach_test detach partition parted_uniq_detach_test1; alter table parted_uniq_detach_test1 drop constraint parted_uniq_detach_test1_a_key; drop table parted_uniq_detach_test, parted_uniq_detach_test1; + +-- check that dropping a column takes with it any partitioned indexes +-- depending on it. +create table parted_index_col_drop(a int, b int, c int) + partition by list (a); +create table parted_index_col_drop1 partition of parted_index_col_drop + for values in (1) partition by list (a); +-- leave this partition without children. +create table parted_index_col_drop2 partition of parted_index_col_drop + for values in (2) partition by list (a); +create table parted_index_col_drop11 partition of parted_index_col_drop1 + for values in (1); +create index on parted_index_col_drop (b); +create index on parted_index_col_drop (c); +create index on parted_index_col_drop (b, c); +alter table parted_index_col_drop drop column c; +\d parted_index_col_drop +\d parted_index_col_drop1 +\d parted_index_col_drop2 +\d parted_index_col_drop11 +drop table parted_index_col_drop;