Re: dropping column prevented due to inherited index
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 signature.asc Description: PGP signature
Re: dropping column prevented due to inherited index
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 signature.asc Description: PGP signature
Re: dropping column prevented due to inherited index
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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: dropping column prevented due to inherited index
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 signature.asc Description: PGP signature
Re: dropping column prevented due to inherited index
On Fri, Oct 11, 2019 at 4:16 PM Michael Paquier 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
Re: dropping column prevented due to inherited index
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(, addrs);
- performDeletion(, 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
Re: dropping column prevented due to inherited index
On Thu, Oct 10, 2019 at 4:53 PM Michael Paquier 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
Re: dropping column prevented due to inherited index
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 signature.asc Description: PGP signature
Re: dropping column prevented due to inherited index
Hello, On Thu, Oct 10, 2019 at 1:13 PM Michael Paquier 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
Re: dropping column prevented due to inherited index
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(, addrs);
- performDeletion(, 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
Re: dropping column prevented due to inherited index
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: dropping column prevented due to inherited index
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
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(, addrs);
- performDeletion(, 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
Re: dropping column prevented due to inherited index
On Tue, Oct 8, 2019 at 2:12 PM Amit Langote wrote: > > Hi Ashutosh, > > On Mon, Oct 7, 2019 at 1:39 PM Ashutosh Sharma 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
Re: dropping column prevented due to inherited index
On Tue, Oct 8, 2019 at 6:15 PM Alvaro Herrera 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
Re: dropping column prevented due to inherited index
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: dropping column prevented due to inherited index
Hi Ashutosh, On Mon, Oct 7, 2019 at 1:39 PM Ashutosh Sharma 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
Re: dropping column prevented due to inherited index
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 On Mon, Oct 7, 2019 at 7:31 AM Amit Langote wrote: > > Hello, > > On Fri, Oct 4, 2019 at 5:57 PM Michael Paquier 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
Re: dropping column prevented due to inherited index
Hello, On Fri, Oct 4, 2019 at 5:57 PM Michael Paquier 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 ATExecDropColumn-inh-recursion-fix_v2.patch Description: Binary data
Re: dropping column prevented due to inherited index
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. -- Michael signature.asc Description: PGP signature
Re: dropping column prevented due to inherited index
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: dropping column prevented due to inherited index
On Wed, Sep 4, 2019 at 2:36 PM Amit Langote 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 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 ATExecDropColumn-inh-recursion-fix.patch Description: Binary data
