Thread: Column/type dependency recording inconsistencies
Hi, While working on an extension upgrade script I got hit by what I believe is a bug in the way we record dependencies between columns and types. CREATE TABLE records dependency between relation and type, not between column and type, but ALTER TABLE ADD COLUMN and ALTER TABLE ALTER COLUMN TYPE record dependencies between relation column and type and not between relation and type Now this might seem harmless (and apparently is most of the time), but there is one case where this breaks things - extensions. If one creates type in an extension and then uses that type for some column in CREATE TABLE statement, everything is fine. But if one then uses that type in either ALTER TABLE ADD COLUMN or ALTER TABLE ALTER COLUMN TYPE then the extension becomes undroppable without CASCADE which is clearly wrong. I attached sample extension that demonstrates this problem. Output of my psql console when creating/dropping this extension: postgres=# create extension deptestext ; CREATE EXTENSION postgres=# drop extension deptestext; ERROR: cannot drop extension deptestext because other objects depend on it DETAIL: table testtable column undroppablecol2 depends on type undroppabletype table testtable column undroppablecol1 depends on type undroppabletype HINT: Use DROP ... CASCADE to drop the dependent objects too. I see two possible fixes for this: - either record relation/type dependency for ALTER TABLE ADD COLUMN and ALTER TABLE ALTER COLUMN TYPE statements instead of column/type one - or record dependency between the column and extension for ALTER TABLE ADD COLUMN and ALTER TABLE ALTER COLUMN TYPE statements Thoughts? Oh and btw looking at the code I think there is same issue with column/collation dependencies. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Petr Jelinek <petr@2ndquadrant.com> writes:
> CREATE TABLE records dependency between relation and type, not between
> column and type, but ALTER TABLE ADD COLUMN and ALTER TABLE ALTER COLUMN
> TYPE record dependencies between relation column and type and not
> between relation and type
Really? I get
regression=# CREATE TYPE droppabletype1 AS (a integer, b text);
CREATE TYPE
regression=# CREATE TABLE testtable (droppablecol1 droppabletype1, undroppablecol1 int);
CREATE TABLE
regression=# CREATE TYPE droppabletype2 AS (a integer, b text);
CREATE TYPE
regression=# alter table testtable add column f3 droppabletype2;
ALTER TABLE
regression=# select pg_describe_object(classid,objid,objsubid) as obj,
pg_describe_object(refclassid,refobjid,refobjsubid)as ref, deptype from pg_depend where refobjid =
'droppabletype1'::regtype; obj | ref | deptype
--------------------------------------+---------------------+---------composite type droppabletype1 | type
droppabletype1| itype droppabletype1[] | type droppabletype1 | itable testtable column droppablecol1 |
typedroppabletype1 | n
(3 rows)
regression=# select pg_describe_object(classid,objid,objsubid) as obj,
pg_describe_object(refclassid,refobjid,refobjsubid)as ref, deptype from pg_depend where refobjid =
'droppabletype2'::regtype; obj | ref | deptype
-------------------------------+---------------------+---------composite type droppabletype2 | type droppabletype2 |
itypedroppabletype2[] | type droppabletype2 | itable testtable column f3 | type droppabletype2 | n
(3 rows)
The dependencies look just the same to me ...
regards, tom lane
On 09/11/14 22:23, Tom Lane wrote: > regression=# select pg_describe_object(classid,objid,objsubid) as obj, pg_describe_object(refclassid,refobjid,refobjsubid)as ref, deptype from pg_depend where refobjid = 'droppabletype1'::regtype; > obj | ref | deptype > --------------------------------------+---------------------+--------- > composite type droppabletype1 | type droppabletype1 | i > type droppabletype1[] | type droppabletype1 | i > table testtable column droppablecol1 | type droppabletype1 | n > (3 rows) > > regression=# select pg_describe_object(classid,objid,objsubid) as obj, pg_describe_object(refclassid,refobjid,refobjsubid)as ref, deptype from pg_depend where refobjid = 'droppabletype2'::regtype; > obj | ref | deptype > -------------------------------+---------------------+--------- > composite type droppabletype2 | type droppabletype2 | i > type droppabletype2[] | type droppabletype2 | i > table testtable column f3 | type droppabletype2 | n > (3 rows) > > The dependencies look just the same to me ... > Hmm actually you are correct, I somehow missed it when I looked manually (I didn't use pg_describe_object). But the problem with the extension persists, I will try to dig more to find what is the real cause. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Petr Jelinek <petr@2ndquadrant.com> writes:
> But the problem with the extension persists, I will try to dig more to
> find what is the real cause.
Hm ... I reproduced the problem here. I can't see anything that looks
wrong about the pg_depend entries:
obj | ref | deptype
----------------------------------------+-------------------------------------------------------+---------extension
deptestext | schema public | ncomposite type droppabletype1
| type droppabletype1 | itype droppabletype1[] | type
droppabletype1 | itype droppabletype1 | schema public
| ntype droppabletype1 | extension deptestext
| etable testtable | schema public | ntable testtable
| extension deptestext | etable testtable column droppablecol2
|type droppabletype1 | ntable testtable column droppablecol1 | type droppabletype1
| ntable testtable column undroppablecol1 | type undroppabletype
| ntable testtable column undroppablecol2 | type undroppabletype | ntype
testtable[] | type testtable | itype testtable
| table testtable | icomposite type undroppabletype | type
undroppabletype | itype undroppabletype[] | type undroppabletype
| itype undroppabletype | schema public
| ntype undroppabletype | extension deptestext | etoast table
pg_toast.pg_toast_162813 | table testtable | itype pg_toast.pg_toast_162813
| toast table pg_toast.pg_toast_162813 | iindex pg_toast.pg_toast_162813_index | toast table
pg_toast.pg_toast_162813column chunk_id | aindex pg_toast.pg_toast_162813_index | toast table
pg_toast.pg_toast_162813column chunk_seq | a
(21 rows)
but sure enough:
d1=# drop extension deptestext;
ERROR: cannot drop extension deptestext because other objects depend on it
DETAIL: table testtable column undroppablecol2 depends on type undroppabletype
table testtable column undroppablecol1 depends on type undroppabletype
HINT: Use DROP ... CASCADE to drop the dependent objects too.
I suspect this is actually a bug in the dependency search logic in DROP.
Don't have the energy to chase it right now though.
regards, tom lane
On 09/11/14 23:04, Tom Lane wrote: > but sure enough: > > d1=# drop extension deptestext; > ERROR: cannot drop extension deptestext because other objects depend on it > DETAIL: table testtable column undroppablecol2 depends on type undroppabletype > table testtable column undroppablecol1 depends on type undroppabletype > HINT: Use DROP ... CASCADE to drop the dependent objects too. > > I suspect this is actually a bug in the dependency search logic in DROP. > Don't have the energy to chase it right now though. > Yes that's where I started searching also and yes it is. The actual situation is that the columns of the table that is about to be dropped are normally not added to the object list that is used for dependency checks (if the table is going to be dropped who cares about what column depends on anyway). But this logic depends on the fact that the columns that belong to that table are processed after the table was already processed, however as the new type was added after the table was added, it's processed before the table and because of the dependency between type and columns, the columns are also processed before the table and so they are added to the object list for further dependency checking. See use and source of object_address_present_add_flags in dependency.c. I am not really sure how to fix this, other than changing this to two step process - essentially go through the resulting object list again and remove the columns that already have table there, but that's not exactly super pretty solution. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 09/11/14 23:36, Petr Jelinek wrote: > On 09/11/14 23:04, Tom Lane wrote: >> >> I suspect this is actually a bug in the dependency search logic in DROP. >> Don't have the energy to chase it right now though. >> > > Yes that's where I started searching also and yes it is. The actual > situation is that the columns of the table that is about to be dropped > are normally not added to the object list that is used for dependency > checks (if the table is going to be dropped who cares about what column > depends on anyway). > But this logic depends on the fact that the columns that belong to that > table are processed after the table was already processed, however as > the new type was added after the table was added, it's processed before > the table and because of the dependency between type and columns, the > columns are also processed before the table and so they are added to the > object list for further dependency checking. > > See use and source of object_address_present_add_flags in dependency.c. > So here is what I came up with to fix this. It's quite simple and works well enough, we don't really have any regression test that would hit this though. I wonder if the object_address_present_add_flags should be renamed since it does bit more than what name suggests at this point. And I think this should be backpatched to all the versions with extension support. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Petr Jelinek <petr@2ndquadrant.com> writes:
> On 09/11/14 23:36, Petr Jelinek wrote:
>> On 09/11/14 23:04, Tom Lane wrote:
>>> I suspect this is actually a bug in the dependency search logic in DROP.
>>> Don't have the energy to chase it right now though.
>> But this logic depends on the fact that the columns that belong to that
>> table are processed after the table was already processed, however as
>> the new type was added after the table was added, it's processed before
>> the table and because of the dependency between type and columns, the
>> columns are also processed before the table and so they are added to the
>> object list for further dependency checking.
Yeah. We had code to handle the table-visited-before-column case, but
for some reason not the column-visited-before-table case. Rather odd
that this hasn't been reported before.
> So here is what I came up with to fix this. It's quite simple and works
> well enough, we don't really have any regression test that would hit
> this though.
This is incomplete. stack_address_present_add_flags needs a similar fix,
and both of them need to be taught to deal with the possibility that more
than one such column is present (I don't think your "continue" quite works
for that). Also, I find physically removing an entry fairly ugly and
possibly unsafe, since it's not clear what outer recursion levels might be
assuming about the contents of the array -- and that certainly won't work
for the stack case. What seems better is to invent another flag bit
to indicate that we no longer need to physically delete this subobject
because we've discovered the whole object will be deleted. Then we
can just skip it during the execution phase.
Will work on this. Thanks for the report and test case!
regards, tom lane
I wrote:
> ... Also, I find physically removing an entry fairly ugly and
> possibly unsafe, since it's not clear what outer recursion levels might be
> assuming about the contents of the array -- and that certainly won't work
> for the stack case. What seems better is to invent another flag bit
> to indicate that we no longer need to physically delete this subobject
> because we've discovered the whole object will be deleted. Then we
> can just skip it during the execution phase.
On further reflection, neither of those approaches is really a good idea.
With either implementation, skipping the DROP COLUMN step would mean that
we're letting the DROP TYPE happen before we drop a table containing a
live column of that type. While that failed to fail in this particular
example, it sounds like a really bad idea --- any number of things, such
as event triggers, could well blow up when abused like that. The lack of
prior reports means that this is a very rare case, so rather than trying
to "optimize" it, I think we should just let the drops happen in the
scheduled order. All that we need is to make sure that the column gets
marked with flags indicating that dropping is allowed. Accordingly, the
attached patch should do it.
regards, tom lane
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 256486c..f338acf 100644
*** a/src/backend/catalog/dependency.c
--- b/src/backend/catalog/dependency.c
*************** object_address_present_add_flags(const O
*** 2116,2121 ****
--- 2116,2122 ----
int flags,
ObjectAddresses *addrs)
{
+ bool result = false;
int i;
for (i = addrs->numrefs - 1; i >= 0; i--)
*************** object_address_present_add_flags(const O
*** 2130,2151 ****
ObjectAddressExtra *thisextra = addrs->extras + i;
thisextra->flags |= flags;
! return true;
}
! if (thisobj->objectSubId == 0)
{
/*
* We get here if we find a need to delete a column after
* having already decided to drop its whole table. Obviously
! * we no longer need to drop the column. But don't plaster
! * its flags on the table.
*/
! return true;
}
}
}
! return false;
}
/*
--- 2131,2178 ----
ObjectAddressExtra *thisextra = addrs->extras + i;
thisextra->flags |= flags;
! result = true;
}
! else if (thisobj->objectSubId == 0)
{
/*
* We get here if we find a need to delete a column after
* having already decided to drop its whole table. Obviously
! * we no longer need to drop the subobject, so report that we
! * found the subobject in the array. But don't plaster its
! * flags on the whole object.
*/
! result = true;
! }
! else if (object->objectSubId == 0)
! {
! /*
! * We get here if we find a need to delete a whole table after
! * having already decided to drop one of its columns. We
! * can't report that the whole object is in the array, but we
! * should mark the subobject with the whole object's flags.
! *
! * It might seem attractive to physically delete the column's
! * array entry, or at least mark it as no longer needing
! * separate deletion. But that could lead to, e.g., dropping
! * the column's datatype before we drop the table, which does
! * not seem like a good idea. This is a very rare situation
! * in practice, so we just take the hit of doing a separate
! * DROP COLUMN action even though we know we're gonna delete
! * the table later.
! *
! * Because there could be other subobjects of this object in
! * the array, this case means we always have to loop through
! * the whole array; we cannot exit early on a match.
! */
! ObjectAddressExtra *thisextra = addrs->extras + i;
!
! thisextra->flags |= flags;
}
}
}
! return result;
}
/*
*************** stack_address_present_add_flags(const Ob
*** 2156,2161 ****
--- 2183,2189 ----
int flags,
ObjectAddressStack *stack)
{
+ bool result = false;
ObjectAddressStack *stackptr;
for (stackptr = stack; stackptr; stackptr = stackptr->next)
*************** stack_address_present_add_flags(const Ob
*** 2168,2188 ****
if (object->objectSubId == thisobj->objectSubId)
{
stackptr->flags |= flags;
! return true;
}
-
- /*
- * Could visit column with whole table already on stack; this is
- * the same case noted in object_address_present_add_flags(), and
- * as in that case, we don't propagate flags for the component to
- * the whole object.
- */
- if (thisobj->objectSubId == 0)
- return true;
}
}
! return false;
}
/*
--- 2196,2226 ----
if (object->objectSubId == thisobj->objectSubId)
{
stackptr->flags |= flags;
! result = true;
! }
! else if (thisobj->objectSubId == 0)
! {
! /*
! * We're visiting a column with whole table already on stack.
! * As in object_address_present_add_flags(), we can skip
! * further processing of the subobject, but we don't want to
! * propagate flags for the subobject to the whole object.
! */
! result = true;
! }
! else if (object->objectSubId == 0)
! {
! /*
! * We're visiting a table with column already on stack. As in
! * object_address_present_add_flags(), we should propagate
! * flags for the whole object to each of its subobjects.
! */
! stackptr->flags |= flags;
}
}
}
! return result;
}
/*
On 11/11/14 21:47, Tom Lane wrote: > I wrote: >> ... Also, I find physically removing an entry fairly ugly and >> possibly unsafe, since it's not clear what outer recursion levels might be >> assuming about the contents of the array -- and that certainly won't work >> for the stack case. What seems better is to invent another flag bit >> to indicate that we no longer need to physically delete this subobject >> because we've discovered the whole object will be deleted. Then we >> can just skip it during the execution phase. > > On further reflection, neither of those approaches is really a good idea. > With either implementation, skipping the DROP COLUMN step would mean that > we're letting the DROP TYPE happen before we drop a table containing a > live column of that type. While that failed to fail in this particular > example, it sounds like a really bad idea --- any number of things, such > as event triggers, could well blow up when abused like that. The lack of > prior reports means that this is a very rare case, so rather than trying > to "optimize" it, I think we should just let the drops happen in the > scheduled order. All that we need is to make sure that the column gets > marked with flags indicating that dropping is allowed. Accordingly, the > attached patch should do it. > Yup, this patch seems to be much better and safer fix. I can confirm that it works fine with both the test-case and my original upgrade script. Thanks! -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services