On Wed, Oct 09, 2019 at 06:36:35AM -0300, Alvaro Herrera wrote: > Right, something like that. Needs a comment to explain what we do and > how recursing=true correlates with addrs=NULL, I think. Maybe add an > assert.
Yes, that would be a thing to do. So I have added more comments regarding that aspect, an assertion, and more tests with a partitioned table without any children, and an actual check that all columns have been dropped in the leaves of the partition tree. How does that look? -- Michael
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 05593f3316..dd5f9cad68 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,
@@ -7004,12 +7007,16 @@ ATPrepDropColumn(List **wqueue, Relation rel, bool recurse, bool recursing,
/*
* Return value is the address of the dropped column.
+ *
+ * *addrs stores the object addresses of all the columns to delete in
+ * case of a recursive lookup on children relations.
*/
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;
@@ -7022,6 +7029,16 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
if (recursing)
ATSimplePermissions(rel, ATT_TABLE | ATT_FOREIGN_TABLE);
+ /*
+ * Initialize the location of addresses which will be deleted on a
+ * recursive lookup on children relations. The structure to store all the
+ * object addresses to delete is initialized once when the recursive
+ * lookup begins.
+ */
+ Assert(!recursing || addrs != NULL);
+ if (!recursing)
+ addrs = new_object_addresses();
+
/*
* get the number of the attribute
*/
@@ -7134,7 +7151,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
{
@@ -7170,14 +7187,20 @@ 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);
+ /*
+ * The recursion is ending, hence perform the actual column deletions.
+ */
+ if (!recursing)
+ {
+ 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..73988c881d 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -1258,3 +1258,54 @@ 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, 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)
+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)
+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)
+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)
+
+drop table parted_index_col_drop;
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index cc3d0abfb7..45eeac3dd6 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -703,3 +703,22 @@ 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, 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;
signature.asc
Description: PGP signature
