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 ba8f4459f3d5ca1a9606ddd1309db84b0d1f1e4b..8d25d147729ae73b1e493aaf05a79c9e47c126a7 100644 (file)
--- 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 c143df5114febc87180e5b0bfb2e4900252dde7f..ec1d4eaef4c5f3ae1d42932e2c3274def68dd662 100644 (file)
--- 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 cc3d0abfb7bf5d78a1b3e45eedd06bede2c67403..f6a376791890a161768ffe3c77e77587e6746d32 100644 (file)
--- 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;