dropping column prevented due to inherited index
Hi,
Maybe I'm forgetting some dependency management discussion that I was
part of recently, but is the following behavior unintentional?
create table p (a int, b int, c int) partition by list (a);
create table p1 partition of p for values in (1) partition by list (b);
create table p11 partition of p1 for values in (1);
create index on p (c);
alter table p drop column c;
ERROR: cannot drop index p11_c_idx because index p1_c_idx requires it
HINT: You can drop index p1_c_idx instead.
Dropping c should've automatically dropped the index p_c_idx and its
children and grandchildren, so the above complaint seems redundant.
Thanks,
Amit
On Wed, Sep 4, 2019 at 2:36 PM Amit Langote <amitlangote09@gmail.com> wrote:
Hi,
Maybe I'm forgetting some dependency management discussion that I was
part of recently, but is the following behavior unintentional?create table p (a int, b int, c int) partition by list (a);
create table p1 partition of p for values in (1) partition by list (b);
create table p11 partition of p1 for values in (1);
create index on p (c);
alter table p drop column c;
ERROR: cannot drop index p11_c_idx because index p1_c_idx requires it
HINT: You can drop index p1_c_idx instead.Dropping c should've automatically dropped the index p_c_idx and its
children and grandchildren, so the above complaint seems redundant.
Interestingly, this behavior only occurs with v12 and HEAD. With v11:
create table p (a int, b int, c int) partition by list (a);
create table p1 partition of p for values in (1) partition by list (b);
create table p11 partition of p1 for values in (1);
create index on p (c);
alter table p drop column c; -- ok
Note that the multi-level partitioning is not really needed to
reproduce the error.
I think the error in my first email has to do with the following
commit made to v12 and HEAD branches earlier this year:
commit 1d92a0c9f7dd547ad14cd8a3974289c5ec7f04ce
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Mon Feb 11 14:41:13 2019 -0500
Redesign the partition dependency mechanism.
There may not really be any problem with the commit itself, but I
suspect that the new types of dependencies (or the way
findDependentObject() analyzes them) don't play well with inheritance
recursion of ATExecDropColumn(). Currently, child columns (and its
dependencies) are dropped before the parent column (and its
dependencies). By using the attached patch which reverses that order,
the error goes away, but I'm not sure that that's the correct
solution.
Tom, thoughts?
Thanks,
Amit
Attachments:
ATExecDropColumn-inh-recursion-fix.patchapplication/octet-stream; name=ATExecDropColumn-inh-recursion-fix.patchDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 05593f3316..679c2ce6a7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7080,6 +7080,15 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
ReleaseSysCache(tuple);
/*
+ * Perform the actual column deletion
+ */
+ object.classId = RelationRelationId;
+ object.objectId = RelationGetRelid(rel);
+ object.objectSubId = attnum;
+
+ performDeletion(&object, behavior, 0);
+
+ /*
* Propagate to children as appropriate. Unlike most other ALTER
* routines, we have to do this one level of recursion at a time; we can't
* use find_all_inheritors to do it in one pass.
@@ -7170,15 +7179,6 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
table_close(attr_rel, RowExclusiveLock);
}
- /*
- * Perform the actual column deletion
- */
- object.classId = RelationRelationId;
- object.objectId = RelationGetRelid(rel);
- object.objectSubId = attnum;
-
- performDeletion(&object, behavior, 0);
-
return object;
}
On 2019-Oct-03, Amit Langote wrote:
There may not really be any problem with the commit itself, but I
suspect that the new types of dependencies (or the way
findDependentObject() analyzes them) don't play well with inheritance
recursion of ATExecDropColumn(). Currently, child columns (and its
dependencies) are dropped before the parent column (and its
dependencies). By using the attached patch which reverses that order,
the error goes away, but I'm not sure that that's the correct
solution.
Hmm. I wonder if we shouldn't adopt the coding pattern we've used
elsewhere of collecting all columns to be dropped first into an
ObjectAddresses array, then use performMultipleDeletions.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Oct 03, 2019 at 09:18:12AM -0300, Alvaro Herrera wrote:
Hmm. I wonder if we shouldn't adopt the coding pattern we've used
elsewhere of collecting all columns to be dropped first into an
ObjectAddresses array, then use performMultipleDeletions.
+1. That's the common pattern these days, because that's more
performant. I think that the patch should have regression tests.
--
MichaelHello,
On Fri, Oct 4, 2019 at 5:57 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Oct 03, 2019 at 09:18:12AM -0300, Alvaro Herrera wrote:
Hmm. I wonder if we shouldn't adopt the coding pattern we've used
elsewhere of collecting all columns to be dropped first into an
ObjectAddresses array, then use performMultipleDeletions.+1. That's the common pattern these days, because that's more
performant.
Actually I don't see the peformMultipleDeletions() pattern being used
for the situations where there are multiple objects to drop due to
inheritance. I only see it where there are multiple objects related
to one table. Maybe it's possible to apply to the inheritance
situation though, but in this particular case, it seems a bit hard to
do, because ATExecDropColumn steps through an inheritance tree level
at a time.
But maybe I misunderstood Alvaro's suggestion?
I think that the patch should have regression tests.
I have added one in the attached updated patch.
Thanks,
Amit
Attachments:
ATExecDropColumn-inh-recursion-fix_v2.patchapplication/octet-stream; name=ATExecDropColumn-inh-recursion-fix_v2.patchDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f31af59ae9..197e914202 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7085,6 +7085,15 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
ReleaseSysCache(tuple);
/*
+ * Perform the actual column deletion
+ */
+ object.classId = RelationRelationId;
+ object.objectId = RelationGetRelid(rel);
+ object.objectSubId = attnum;
+
+ performDeletion(&object, behavior, 0);
+
+ /*
* Propagate to children as appropriate. Unlike most other ALTER
* routines, we have to do this one level of recursion at a time; we can't
* use find_all_inheritors to do it in one pass.
@@ -7175,15 +7184,6 @@ ATExecDropColumn(List **wqueue, Relation rel, const char *colName,
table_close(attr_rel, RowExclusiveLock);
}
- /*
- * Perform the actual column deletion
- */
- object.classId = RelationRelationId;
- object.objectId = RelationGetRelid(rel);
- object.objectSubId = attnum;
-
- performDeletion(&object, behavior, 0);
-
return object;
}
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index c143df5114..28498b220d 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -1258,3 +1258,20 @@ 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);
+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 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: 1 (Use \d+ to list them.)
+
+drop table parted_index_col_drop;
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index cc3d0abfb7..8ba054b907 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -703,3 +703,13 @@ 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);
+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 c;
+\d parted_index_col_drop
+drop table parted_index_col_drop;
I think we could have first deleted all the dependency of child object
on parent and then deleted the child itself using performDeletion().
As an example let's consider the case of toast table where we first
delete the dependency of toast relation on main relation and then
delete the toast table itself otherwise the toast table deletion would
fail. But, the problem I see here is that currently we do not have any
entry in pg_attribute table that would tell us about the dependency of
child column on parent.
--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com
Show quoted text
On Mon, Oct 7, 2019 at 7:31 AM Amit Langote <amitlangote09@gmail.com> wrote:
Hello,
On Fri, Oct 4, 2019 at 5:57 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Oct 03, 2019 at 09:18:12AM -0300, Alvaro Herrera wrote:
Hmm. I wonder if we shouldn't adopt the coding pattern we've used
elsewhere of collecting all columns to be dropped first into an
ObjectAddresses array, then use performMultipleDeletions.+1. That's the common pattern these days, because that's more
performant.Actually I don't see the peformMultipleDeletions() pattern being used
for the situations where there are multiple objects to drop due to
inheritance. I only see it where there are multiple objects related
to one table. Maybe it's possible to apply to the inheritance
situation though, but in this particular case, it seems a bit hard to
do, because ATExecDropColumn steps through an inheritance tree level
at a time.But maybe I misunderstood Alvaro's suggestion?
I think that the patch should have regression tests.
I have added one in the attached updated patch.
Thanks,
Amit
Hi Ashutosh,
On Mon, Oct 7, 2019 at 1:39 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
I think we could have first deleted all the dependency of child object
on parent and then deleted the child itself using performDeletion().
So, there are two objects to consider in this case -- column and an
index that depends on it.
For columns, we don't store any dependency records for the dependency
between a child column and its corresponding parent column. That
dependency is explicitly managed by the code and the attinhcount flag,
which if set, prevents the column from being dropped on its own.
Indexes do rely on dependency records for the dependency between a
child index and its corresponding parent index. This dependency
prevents a child index from being dropped if the corresponding parent
index is also not being dropped.
In this case, recursively dropping a child's column will in turn try
to drop the depending child index. findDependentObject() complains
because it can't allow a child index to be dropped, because it can't
establish that the corresponding parent index is also being dropped.
That's because the parent index will be dropped when the parent's
column will be dropped, which due to current coding of
ATExecDropColumn() is *after* all the child columns and indexes are
dropped. If we drop the parent column and depending index first and
then recurse to children as my proposed patch does, then the parent
index would already have been dropped when dropping the child column
and the depending index.
As an example let's consider the case of toast table where we first
delete the dependency of toast relation on main relation and then
delete the toast table itself otherwise the toast table deletion would
fail. But, the problem I see here is that currently we do not have any
entry in pg_attribute table that would tell us about the dependency of
child column on parent.
I couldn't imagine how that trick could be implemented for this case. :(
Thanks,
Amit
Apologies for not helping much here; I'm on vacation for a couple of
weeks.
On 2019-Oct-08, Amit Langote wrote:
I couldn't imagine how that trick could be implemented for this case. :(
Can't we pass around an ObjectAddresses pointer to which each recursion
level adds the object(s) it wants to delete, and in the outermost level
we drop everything in it? I vaguely recall we implemented something
like that somewhere within the past year, but all I can find is
20bef2c3110a.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Oct 8, 2019 at 6:15 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
Apologies for not helping much here; I'm on vacation for a couple of
weeks.
No worries, please take your time.
On 2019-Oct-08, Amit Langote wrote:
I couldn't imagine how that trick could be implemented for this case. :(
Can't we pass around an ObjectAddresses pointer to which each recursion
level adds the object(s) it wants to delete, and in the outermost level
we drop everything in it? I vaguely recall we implemented something
like that somewhere within the past year, but all I can find is
20bef2c3110a.
I thought about doing something like that, but wasn't sure if
introducing that much complexity is warranted.
Thanks,
Amit
On Tue, Oct 8, 2019 at 2:12 PM Amit Langote <amitlangote09@gmail.com> wrote:
Hi Ashutosh,
On Mon, Oct 7, 2019 at 1:39 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
I think we could have first deleted all the dependency of child object
on parent and then deleted the child itself using performDeletion().So, there are two objects to consider in this case -- column and an
index that depends on it.For columns, we don't store any dependency records for the dependency
between a child column and its corresponding parent column. That
dependency is explicitly managed by the code and the attinhcount flag,
which if set, prevents the column from being dropped on its own.Indexes do rely on dependency records for the dependency between a
child index and its corresponding parent index. This dependency
prevents a child index from being dropped if the corresponding parent
index is also not being dropped.In this case, recursively dropping a child's column will in turn try
to drop the depending child index. findDependentObject() complains
because it can't allow a child index to be dropped, because it can't
establish that the corresponding parent index is also being dropped.
That's because the parent index will be dropped when the parent's
column will be dropped, which due to current coding of
ATExecDropColumn() is *after* all the child columns and indexes are
dropped. If we drop the parent column and depending index first and
then recurse to children as my proposed patch does, then the parent
index would already have been dropped when dropping the child column
and the depending index.As an example let's consider the case of toast table where we first
delete the dependency of toast relation on main relation and then
delete the toast table itself otherwise the toast table deletion would
fail. But, the problem I see here is that currently we do not have any
entry in pg_attribute table that would tell us about the dependency of
child column on parent.I couldn't imagine how that trick could be implemented for this case. :(
I don't think that is possible because presently in pg_attribute table
we do not have any column indicating that there exists index on the
given attribute. If we were knowing that then we could find out if the
given index on child table have been inherited from parent and if so,
we could delete all the dependencies on the child table index first
and then delete the column itself in the child table but that doesn't
seem to be doable here. So, although the standard way that I feel to
perform an object deletion is to first remove all it's dependencies
from the pg_depend table and then delete the object itself but
considering the information available in the relevant catalog table
that doesn't seem to be possible.
--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com
--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com
On Tue, Oct 08, 2019 at 06:25:05PM +0900, Amit Langote wrote:
I thought about doing something like that, but wasn't sure if
introducing that much complexity is warranted.
I looked at that. By experience, I think that it would be wiser to do
first the lookup of all the dependencies you would like to delete, and
then let the internal dependency machinery sort things out after
recursing (remember recent fixes related to ON COMMIT actions). In
order to do that, you actually just need to be careful to not trigger
the deletions as long as "recursing" is true because ATExecDropColumn
calls itself. And it is not actually as bad as I assumed, please see
the attached.
--
Michael
Attachments:
ATExecDropColumn-inh-recursion-fix_v3.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 05593f3316..87c39ad6df 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 recursing lookup.
*/
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,13 @@ 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
+ * recursing inheritance lookup.
+ */
+ if (!recursing)
+ addrs = new_object_addresses();
+
/*
* get the number of the attribute
*/
@@ -7134,7 +7148,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 +7184,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..28498b220d 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -1258,3 +1258,20 @@ 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);
+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 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: 1 (Use \d+ to list them.)
+
+drop table parted_index_col_drop;
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index cc3d0abfb7..8ba054b907 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -703,3 +703,13 @@ 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);
+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 c;
+\d parted_index_col_drop
+drop table parted_index_col_drop;
On 2019-Oct-09, Michael Paquier wrote:
On Tue, Oct 08, 2019 at 06:25:05PM +0900, Amit Langote wrote:
I thought about doing something like that, but wasn't sure if
introducing that much complexity is warranted.I looked at that. By experience, I think that it would be wiser to do
first the lookup of all the dependencies you would like to delete, and
then let the internal dependency machinery sort things out after
recursing (remember recent fixes related to ON COMMIT actions). In
order to do that, you actually just need to be careful to not trigger
the deletions as long as "recursing" is true because ATExecDropColumn
calls itself. And it is not actually as bad as I assumed, please see
the attached.
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.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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
Attachments:
ATExecDropColumn-inh-recursion-fix_v4.patchtext/x-diff; charset=us-asciiDownload
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;
Hello,
On Thu, Oct 10, 2019 at 1:13 PM Michael Paquier <michael@paquier.xyz> wrote:
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?
Thanks for the patch. I think we should test there being multiple
dependent indexes on the column being dropped; the patch currently
tests only one.
Comments on comments: ;)
+ *
+ * *addrs stores the object addresses of all the columns to delete in
+ * case of a recursive lookup on children relations.I think this comment fails to make clear the state of addrs in a given
invocation of ATExecDropColumn(). Maybe the whole header comment
could be rewritten to explain this function's new mode of operation.
How about this:
/*
* 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.
*/
/*
+ * 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();Maybe this could just say:
/* Initialize addrs on the first invocation. */
+ /*
+ * The recursion is ending, hence perform the actual column deletions.
+ */Maybe:
/* All columns to be dropped must now be in addrs, so drop. */
Thanks,
Amit
On Thu, Oct 10, 2019 at 02:56:32PM +0900, Amit Langote wrote:
/*
* 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.
*/
Sounds fine to me.
+ * 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();Maybe this could just say:
/* Initialize addrs on the first invocation. */
I would add "recursive" here, to give:
/* Initialize addrs on the first recursive invocation. */
+ /* + * The recursion is ending, hence perform the actual column deletions. + */Maybe:
/* All columns to be dropped must now be in addrs, so drop. */
I think that it would be better to clarify as well that this stands
after all the child relations have been checked, so what about that?
"The resursive 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."
--
Michael
On Thu, Oct 10, 2019 at 4:53 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Oct 10, 2019 at 02:56:32PM +0900, Amit Langote wrote:
/* Initialize addrs on the first invocation. */
I would add "recursive" here, to give:
/* Initialize addrs on the first recursive invocation. */
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.
+ /* + * The recursion is ending, hence perform the actual column deletions. + */Maybe:
/* All columns to be dropped must now be in addrs, so drop. */
I think that it would be better to clarify as well that this stands
after all the child relations have been checked, so what about that?
"The resursive 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."
Okay, sure. Maybe it's better to write the comment inside the if
block, because if recursing is true, we don't drop yet.
Thoughts on suggestion to expand the test case?
Thanks,
Amit
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
Attachments:
ATExecDropColumn-inh-recursion-fix_v5.patchtext/x-diff; charset=us-asciiDownload
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;
On Fri, Oct 11, 2019 at 4:16 PM Michael Paquier <michael@paquier.xyz> wrote:
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?
Thanks. The index on b is not really necessary for testing because it
remains unaffected, but maybe it's fine.
Regards,
Amit
On Fri, Oct 11, 2019 at 04:23:51PM +0900, Amit Langote wrote:
Thanks. The index on b is not really necessary for testing because it
remains unaffected, but maybe it's fine.
That's on purpose. Any more comments?
--
Michael
On 2019-Oct-11, Michael Paquier wrote:
+ 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); + }
Typo "resursing". This comment seems a bit too long to me. Maybe
"Recursion having ended, drop everything that was collected." suffices.
(Fits in one line.)
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Oct 11, 2019 at 06:39:47PM -0300, Alvaro Herrera wrote:
Typo "resursing". This comment seems a bit too long to me. Maybe
"Recursion having ended, drop everything that was collected." suffices.
(Fits in one line.)
Sounds fine to me, thanks.
--
Michael
On Sat, Oct 12, 2019 at 03:08:41PM +0900, Michael Paquier wrote:
On Fri, Oct 11, 2019 at 06:39:47PM -0300, Alvaro Herrera wrote:
Typo "resursing". This comment seems a bit too long to me. Maybe
"Recursion having ended, drop everything that was collected." suffices.
(Fits in one line.)Sounds fine to me, thanks.
I have been able to look at this issue once again, and applied the fix
down to v12. Thanks, all!
--
Michael