On Thu, Oct 10, 2019 at 05:28:02PM +0900, Amit Langote wrote: > Actually, the code initializes it on the first call (recursing is > false) and asserts that it must have been already initialized in a > recursive (recursing is true) call.
I have actually kept your simplified version. > Okay, sure. Maybe it's better to write the comment inside the if > block, because if recursing is true, we don't drop yet. Sure. > Thoughts on suggestion to expand the test case? No objections to that, so done as per the attached. Does that match what you were thinking about? -- Michael
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ba8f4459f3..74c0a00a2f 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,
@@ -4893,8 +4896,8 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
/*
* Set all columns in the new slot to NULL initially, to ensure
- * columns added as part of the rewrite are initialized to
- * NULL. That is necessary as tab->newvals will not contain an
+ * columns added as part of the rewrite are initialized to NULL.
+ * That is necessary as tab->newvals will not contain an
* expression for columns with a NULL default, e.g. when adding a
* column without a default together with a column with a default
* requiring an actual rewrite.
@@ -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,22 @@ 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)
+ {
+ /*
+ * The resursing lookup for inherited child relations is done. All
+ * the child relations have been scanned and the object addresses of
+ * all the columns to-be-dropped are registered in addrs, so drop.
+ */
+ 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;
signature.asc
Description: PGP signature
