Обсуждение: Another extensions bug
Whilst testing the schema-creation-permissions scenario that Kaigai-san
recently pointed out, I happened to do this:
regression=# create schema c;
CREATE SCHEMA
regression=# create extension cube with schema c;
CREATE EXTENSION
regression=# drop schema c;
DROP SCHEMA
... er, what? I was expecting
ERROR: cannot drop schema c because other objects depend on it
DETAIL: extension cube depends on schema c
HINT: Use DROP ... CASCADE to drop the dependent objects too.
Inspection of pg_depend proved that indeed there was only a NORMAL
dependency of the extension on the schema, so this should not have
happened.
After a good bit of careful tracing, I came to the conclusion that the
bug is here in findDependentObjects, when it comes across an INTERNAL or
EXTENSION dependency in its first loop:
/* * Okay, recurse to the other object instead of proceeding. We * treat
thisexactly as if the original reference had linked * to that object instead of this one; hence, pass
throughthe * same flags and stack. */ findDependentObjects(&otherObject,
flags, stack,
targetObjects, pendingObjects, depRel);
What happens in the cube case is that there's an INTERNAL dependency
from type cube[] to type cube, while both of these types also have
EXTENSION dependencies on the cube extension. So, when we trace the
INTERNAL dependency from type cube to type cube[], findDependentObjects
is entered with target object cube[] and flags = DEPFLAG_INTERNAL. In
its first loop, it discovers that cube[] has an EXTENSION dependency on
the cube extension, so the above code recurses to the extension object
with flags = DEPFLAG_INTERNAL. And that causes the extension to get
marked as being internally dependent on some other object-to-be-deleted,
which then licenses the dependency code to delete it silently and
without requiring CASCADE. Ooops.
I think the above-quoted code is just fundamentally wrong. It was never
possible to reach that code with flags = DEPFLAG_INTERNAL before,
because an object can have only one INTERNAL dependency and it's the one
that would have been caught by the preceding test to see if we were
recursing from the other end of that dependency. Now that we've added
the possibility of EXTENSION dependencies, we could get here for an
EXTENSION dependency after recursing from an INTERNAL dependency (as in
this case), or for an INTERNAL dependency after recursing from an
EXTENSION dependency. In neither of those cases does it make sense to
propagate the previous flags state. It is conceivable that we should
allow AUTO dependencies to propagate here, but I'm unconvinced; I don't
think the case can ever arise at present, and it certainly couldn't have
arisen before (there is no case where the system will label an object
with both AUTO and INTERNAL dependencies). If there is a reason to do
an AUTO drop of the referenced object, it should show up in some other
dependency of that object. So I'm thinking this recursive call should
just pass DEPFLAG_NORMAL in all cases:
findDependentObjects(&otherObject, DEPFLAG_NORMAL,
stack, targetObjects,
pendingObjects, depRel);
I've not tested this heavily, but it fixes the example above and it
passes the regression tests.
Comments?
regards, tom lane
I wrote:
> ... So I'm thinking this recursive call should
> just pass DEPFLAG_NORMAL in all cases:
On further reflection, it seems more in keeping with the coding
elsewhere in this module to treat this as a distinct dependency type,
instead of confusing it with a NORMAL dependency. There's no actual
functional difference at the moment, but more info is better than less.
Hence, proposed patch attached (which also improves some of the related
comments).
regards, tom lane
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index c459c1e221383b47a6d4b21178b6b4d9e9b5f471..0e6165118556ca47b494ff2674f83016548a3fc8 100644
*** a/src/backend/catalog/dependency.c
--- b/src/backend/catalog/dependency.c
*************** typedef struct
*** 98,103 ****
--- 98,104 ----
#define DEPFLAG_AUTO 0x0004 /* reached via auto dependency */
#define DEPFLAG_INTERNAL 0x0008 /* reached via internal dependency */
#define DEPFLAG_EXTENSION 0x0010 /* reached via extension dependency */
+ #define DEPFLAG_REVERSE 0x0020 /* reverse internal/extension link */
/* expansible list of ObjectAddresses */
*************** findDependentObjects(const ObjectAddress
*** 513,524 ****
/*
* The target object might be internally dependent on some other object
! * (its "owner"). If so, and if we aren't recursing from the owning
! * object, we have to transform this deletion request into a deletion
! * request of the owning object. (We'll eventually recurse back to this
! * object, but the owning object has to be visited first so it will be
! * deleted after.) The way to find out about this is to scan the
! * pg_depend entries that show what this object depends on.
*/
ScanKeyInit(&key[0],
Anum_pg_depend_classid,
--- 514,526 ----
/*
* The target object might be internally dependent on some other object
! * (its "owner"), or be a member of an extension (again considered its
! * owner). If so, and if we aren't recursing from the owning object, we
! * have to transform this deletion request into a deletion request of the
! * owning object. (We'll eventually recurse back to this object, but the
! * owning object has to be visited first so it will be deleted after.)
! * The way to find out about this is to scan the pg_depend entries that
! * show what this object depends on.
*/
ScanKeyInit(&key[0],
Anum_pg_depend_classid,
*************** findDependentObjects(const ObjectAddress
*** 567,573 ****
* 1. At the outermost recursion level, disallow the DROP. (We
* just ereport here, rather than proceeding, since no other
* dependencies are likely to be interesting.) However, if
! * the other object is listed in pendingObjects, just release
* the caller's lock and return; we'll eventually complete the
* DROP when we reach that entry in the pending list.
*/
--- 569,575 ----
* 1. At the outermost recursion level, disallow the DROP. (We
* just ereport here, rather than proceeding, since no other
* dependencies are likely to be interesting.) However, if
! * the owning object is listed in pendingObjects, just release
* the caller's lock and return; we'll eventually complete the
* DROP when we reach that entry in the pending list.
*/
*************** findDependentObjects(const ObjectAddress
*** 607,625 ****
/*
* 3. When recursing from anyplace else, transform this
! * deletion request into a delete of the other object.
*
* First, release caller's lock on this object and get
! * deletion lock on the other object. (We must release
* caller's lock to avoid deadlock against a concurrent
! * deletion of the other object.)
*/
ReleaseDeletionLock(object);
AcquireDeletionLock(&otherObject);
/*
! * The other object might have been deleted while we waited to
! * lock it; if so, neither it nor the current object are
* interesting anymore. We test this by checking the
* pg_depend entry (see notes below).
*/
--- 609,627 ----
/*
* 3. When recursing from anyplace else, transform this
! * deletion request into a delete of the owning object.
*
* First, release caller's lock on this object and get
! * deletion lock on the owning object. (We must release
* caller's lock to avoid deadlock against a concurrent
! * deletion of the owning object.)
*/
ReleaseDeletionLock(object);
AcquireDeletionLock(&otherObject);
/*
! * The owning object might have been deleted while we waited
! * to lock it; if so, neither it nor the current object are
* interesting anymore. We test this by checking the
* pg_depend entry (see notes below).
*/
*************** findDependentObjects(const ObjectAddress
*** 631,643 ****
}
/*
! * Okay, recurse to the other object instead of proceeding. We
! * treat this exactly as if the original reference had linked
! * to that object instead of this one; hence, pass through the
! * same flags and stack.
*/
findDependentObjects(&otherObject,
! flags,
stack,
targetObjects,
pendingObjects,
--- 633,650 ----
}
/*
! * Okay, recurse to the owning object instead of proceeding.
! *
! * We do not need to stack the current object; we want the
! * traversal order to be as if the original reference had
! * linked to the owning object instead of this one.
! *
! * The dependency type is a "reverse" dependency: we need to
! * delete the owning object if this one is to be deleted, but
! * this linkage is never a reason for an automatic deletion.
*/
findDependentObjects(&otherObject,
! DEPFLAG_REVERSE,
stack,
targetObjects,
pendingObjects,
Tom Lane <tgl@sss.pgh.pa.us> writes: > On further reflection, it seems more in keeping with the coding > elsewhere in this module to treat this as a distinct dependency type, > instead of confusing it with a NORMAL dependency. There's no actual > functional difference at the moment, but more info is better than less. Seems better indeed. In my first implementation, we had no EXTENSION kind of dependency and used only INTERNAL, which IIRC reads reverse than the other ones. Having to finally have EXTENSION and REVERSE kinds of dependencies here is not that surprising. > Hence, proposed patch attached (which also improves some of the related > comments). +1 on the idea, although I'm not in a position to further review or play with the patch today. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> Hence, proposed patch attached (which also improves some of the related
>> comments).
> +1 on the idea, although I'm not in a position to further review or play
> with the patch today.
Further testing shows that this patch still doesn't make things really
work properly. There's yet *another* bug with extension-dropping: given
a case such as types cube[] and cube both belonging to extension cube,
the end result is that type cube[] never gets dropped at all! This is
because, since cube[] has both INTERNAL and EXTENSION dependencies, it's
impossible for findDependentObjects to get past its first loop when
cube[] is the target. It will always recurse to the other owning
object. Infinite recursion is avoided because it finds the other owning
object already on the stack, but we never get to the point of deciding
that it's okay to delete type cube[]. So:
test=# create extension cube;
CREATE EXTENSION
test=# \dT List of data typesSchema | Name |
Description
--------+------+----------------------------------------------------------------
-----------------------------public | cube | multi-dimensional cube '(FLOAT-1, FLOAT-2, ..., FLOAT-N), (FLOA
T-1, FLOAT-2, ..., FLOAT-N)'
(1 row)
test=# drop extension cube;
DROP EXTENSION
test=# \dT List of data typesSchema | Name | Description
--------+-------+-------------public | ???[] |
(1 row)
test=#
This is another bug that was arguably latent in the original coding,
but could not manifest as long as there was only one INTERNAL-like
dependency per object.
I think the right fix is to replace the simple "is the referenced object
on top of the stack?" check at lines 600ff with a check for "is the
referenced object anywhere in the stack?".
It's also becoming brutally obvious that we need some regression tests
checking extension drop scenarios. I'll work on that today too.
regards, tom lane