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
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 | 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 ...
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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 | n
composite type droppabletype1 | type droppabletype1 | i
type droppabletype1[] | type droppabletype1 | i
type droppabletype1 | schema public | n
type droppabletype1 | extension deptestext | e
table testtable | schema public | n
table testtable | extension deptestext | e
table testtable column droppablecol2 | type droppabletype1 | n
table testtable column droppablecol1 | type droppabletype1 | n
table testtable column undroppablecol1 | type undroppabletype | n
table testtable column undroppablecol2 | type undroppabletype | n
type testtable[] | type testtable | i
type testtable | table testtable | i
composite type undroppabletype | type undroppabletype | i
type undroppabletype[] | type undroppabletype | i
type undroppabletype | schema public | n
type undroppabletype | extension deptestext | e
toast table pg_toast.pg_toast_162813 | table testtable | i
type pg_toast.pg_toast_162813 | toast table pg_toast.pg_toast_162813 | i
index pg_toast.pg_toast_162813_index | toast table pg_toast.pg_toast_162813 column chunk_id | a
index pg_toast.pg_toast_162813_index | toast table pg_toast.pg_toast_162813 column 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
remove_subobjs_from_deplist-v0.patchtext/x-diff; name=remove_subobjs_from_deplist-v0.patchDownload
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 256486c..e7ec7e2 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -2142,6 +2142,31 @@ object_address_present_add_flags(const ObjectAddress *object,
*/
return true;
}
+ if (object->objectSubId == 0)
+ {
+ /*
+ * If we got here, it means that we are dropping table whose
+ * columns are already in the object list, since we assume
+ * later on that there are no subobjects of object present
+ * in the list, we should remove those.
+ *
+ * Note that this is very rare occasion that normally happens
+ * only when dropping extension which had some of its table
+ * columns ALTERed after CREATE with custom types or
+ * collations.
+ */
+ if (i < addrs->numrefs - 1)
+ {
+ ObjectAddressExtra *thisextra = addrs->extras + i;
+
+ memmove(thisobj, thisobj + 1,
+ (addrs->numrefs - 1 - i) * sizeof(ObjectAddress));
+ memmove(thisextra, thisextra + 1,
+ (addrs->numrefs - 1 - i) * sizeof(ObjectAddressExtra));
+ }
+ addrs->numrefs--;
+ continue;
+ }
}
}
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
Attachments:
drop-column-before-table.patchtext/x-diff; charset=us-ascii; name=drop-column-before-table.patchDownload
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers