From cb5c2ba2d82688d29b5902d86b993a54355cad4d Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 24 Aug 2011 13:09:06 -0400 Subject: [PATCH] Fix multiple bugs in extension dropping. When we implemented extensions, we made findDependentObjects() treat EXTENSION dependency links similarly to INTERNAL links. However, that logic contained an implicit assumption that an object could have at most one INTERNAL dependency, so it did not work correctly for objects having both INTERNAL and DEPENDENCY links. This led to failure to drop some extension member objects when dropping the extension. Furthermore, we'd never actually exercised the case of recursing to an internally-referenced (owning) object from anything other than a NORMAL dependency, and it turns out that passing the incoming dependency's flags to the owning object is the Wrong Thing. This led to sometimes dropping a whole extension silently when we should have rejected the drop command for lack of CASCADE. Since we obviously were under-testing extension drop scenarios, add some regression test cases. Unfortunately, such test cases require some extensions (duh), so we can't test for problems in the core regression tests. I chose to add them to the earthdistance contrib module, which is a good test case because it has a dependency on the cube contrib module. Back-patch to 9.1. Arguably these are pre-existing bugs in INTERNAL dependency handling, but since it appears that the cases can never arise pre-9.1, I'll refrain from back-patching the logic changes further than that. --- .../earthdistance/expected/earthdistance.out | 332 +++++++++++++++++- contrib/earthdistance/sql/earthdistance.sql | 74 +++- src/backend/catalog/dependency.c | 119 ++++--- 3 files changed, 477 insertions(+), 48 deletions(-) diff --git a/contrib/earthdistance/expected/earthdistance.out b/contrib/earthdistance/expected/earthdistance.out index 8a3fc749c1..e9382a39a4 100644 --- a/contrib/earthdistance/expected/earthdistance.out +++ b/contrib/earthdistance/expected/earthdistance.out @@ -1,6 +1,14 @@ -- --- Test earth distance functions +-- Test earthdistance extension -- +-- In this file we also do some testing of extension create/drop scenarios. +-- That's really exercising the core database's dependency logic, so ideally +-- we'd do it in the core regression tests, but we can't for lack of suitable +-- guaranteed-available extensions. earthdistance is a good test case because +-- it has a dependency on the cube extension. +-- +CREATE EXTENSION earthdistance; -- fail, must install cube first +ERROR: required extension "cube" is not installed CREATE EXTENSION cube; CREATE EXTENSION earthdistance; -- @@ -945,3 +953,325 @@ SELECT abs(cube_distance(ll_to_earth(-30,-90), '(0)'::cube) / earth() - 1) < t (1 row) +-- +-- Now we are going to test extension create/drop scenarios. +-- +-- list what's installed +\dT + List of data types + Schema | Name | Description +--------+-------+--------------------------------------------------------------------------------------------- + public | cube | multi-dimensional cube '(FLOAT-1, FLOAT-2, ..., FLOAT-N), (FLOAT-1, FLOAT-2, ..., FLOAT-N)' + public | earth | +(2 rows) + +\df + List of functions + Schema | Name | Result data type | Argument data types | Type +--------+-------------------+------------------+------------------------------------------+-------- + public | cube | cube | cube, double precision | normal + public | cube | cube | cube, double precision, double precision | normal + public | cube | cube | double precision | normal + public | cube | cube | double precision, double precision | normal + public | cube | cube | double precision[] | normal + public | cube | cube | double precision[], double precision[] | normal + public | cube_cmp | integer | cube, cube | normal + public | cube_contained | boolean | cube, cube | normal + public | cube_contains | boolean | cube, cube | normal + public | cube_dim | integer | cube | normal + public | cube_distance | double precision | cube, cube | normal + public | cube_enlarge | cube | cube, double precision, integer | normal + public | cube_eq | boolean | cube, cube | normal + public | cube_ge | boolean | cube, cube | normal + public | cube_gt | boolean | cube, cube | normal + public | cube_in | cube | cstring | normal + public | cube_inter | cube | cube, cube | normal + public | cube_is_point | boolean | cube | normal + public | cube_le | boolean | cube, cube | normal + public | cube_ll_coord | double precision | cube, integer | normal + public | cube_lt | boolean | cube, cube | normal + public | cube_ne | boolean | cube, cube | normal + public | cube_out | cstring | cube | normal + public | cube_overlap | boolean | cube, cube | normal + public | cube_size | double precision | cube | normal + public | cube_subset | cube | cube, integer[] | normal + public | cube_union | cube | cube, cube | normal + public | cube_ur_coord | double precision | cube, integer | normal + public | earth | double precision | | normal + public | earth_box | cube | earth, double precision | normal + public | earth_distance | double precision | earth, earth | normal + public | g_cube_compress | internal | internal | normal + public | g_cube_consistent | boolean | internal, cube, integer, oid, internal | normal + public | g_cube_decompress | internal | internal | normal + public | g_cube_penalty | internal | internal, internal, internal | normal + public | g_cube_picksplit | internal | internal, internal | normal + public | g_cube_same | internal | cube, cube, internal | normal + public | g_cube_union | cube | internal, internal | normal + public | gc_to_sec | double precision | double precision | normal + public | geo_distance | double precision | point, point | normal + public | latitude | double precision | earth | normal + public | ll_to_earth | earth | double precision, double precision | normal + public | longitude | double precision | earth | normal + public | sec_to_gc | double precision | double precision | normal +(44 rows) + +\do + List of operators + Schema | Name | Left arg type | Right arg type | Result type | Description +--------+------+---------------+----------------+------------------+-------------------------- + public | && | cube | cube | boolean | overlaps + public | < | cube | cube | boolean | lower than + public | <= | cube | cube | boolean | lower than or equal to + public | <> | cube | cube | boolean | different + public | <@ | cube | cube | boolean | contained in + public | <@> | point | point | double precision | + public | = | cube | cube | boolean | same as + public | > | cube | cube | boolean | greater than + public | >= | cube | cube | boolean | greater than or equal to + public | @ | cube | cube | boolean | contains + public | @> | cube | cube | boolean | contains + public | ~ | cube | cube | boolean | contained in +(12 rows) + +drop extension cube; -- fail, earthdistance requires it +ERROR: cannot drop extension cube because other objects depend on it +DETAIL: extension earthdistance depends on extension cube +HINT: Use DROP ... CASCADE to drop the dependent objects too. +drop extension earthdistance; +drop type cube; -- fail, extension cube requires it +ERROR: cannot drop type cube because extension cube requires it +HINT: You can drop extension cube instead. +-- list what's installed +\dT + List of data types + Schema | Name | Description +--------+------+--------------------------------------------------------------------------------------------- + public | cube | multi-dimensional cube '(FLOAT-1, FLOAT-2, ..., FLOAT-N), (FLOAT-1, FLOAT-2, ..., FLOAT-N)' +(1 row) + +\df + List of functions + Schema | Name | Result data type | Argument data types | Type +--------+-------------------+------------------+------------------------------------------+-------- + public | cube | cube | cube, double precision | normal + public | cube | cube | cube, double precision, double precision | normal + public | cube | cube | double precision | normal + public | cube | cube | double precision, double precision | normal + public | cube | cube | double precision[] | normal + public | cube | cube | double precision[], double precision[] | normal + public | cube_cmp | integer | cube, cube | normal + public | cube_contained | boolean | cube, cube | normal + public | cube_contains | boolean | cube, cube | normal + public | cube_dim | integer | cube | normal + public | cube_distance | double precision | cube, cube | normal + public | cube_enlarge | cube | cube, double precision, integer | normal + public | cube_eq | boolean | cube, cube | normal + public | cube_ge | boolean | cube, cube | normal + public | cube_gt | boolean | cube, cube | normal + public | cube_in | cube | cstring | normal + public | cube_inter | cube | cube, cube | normal + public | cube_is_point | boolean | cube | normal + public | cube_le | boolean | cube, cube | normal + public | cube_ll_coord | double precision | cube, integer | normal + public | cube_lt | boolean | cube, cube | normal + public | cube_ne | boolean | cube, cube | normal + public | cube_out | cstring | cube | normal + public | cube_overlap | boolean | cube, cube | normal + public | cube_size | double precision | cube | normal + public | cube_subset | cube | cube, integer[] | normal + public | cube_union | cube | cube, cube | normal + public | cube_ur_coord | double precision | cube, integer | normal + public | g_cube_compress | internal | internal | normal + public | g_cube_consistent | boolean | internal, cube, integer, oid, internal | normal + public | g_cube_decompress | internal | internal | normal + public | g_cube_penalty | internal | internal, internal, internal | normal + public | g_cube_picksplit | internal | internal, internal | normal + public | g_cube_same | internal | cube, cube, internal | normal + public | g_cube_union | cube | internal, internal | normal +(35 rows) + +\do + List of operators + Schema | Name | Left arg type | Right arg type | Result type | Description +--------+------+---------------+----------------+-------------+-------------------------- + public | && | cube | cube | boolean | overlaps + public | < | cube | cube | boolean | lower than + public | <= | cube | cube | boolean | lower than or equal to + public | <> | cube | cube | boolean | different + public | <@ | cube | cube | boolean | contained in + public | = | cube | cube | boolean | same as + public | > | cube | cube | boolean | greater than + public | >= | cube | cube | boolean | greater than or equal to + public | @ | cube | cube | boolean | contains + public | @> | cube | cube | boolean | contains + public | ~ | cube | cube | boolean | contained in +(11 rows) + +create table foo (f1 cube, f2 int); +drop extension cube; -- fail, foo.f1 requires it +ERROR: cannot drop extension cube because other objects depend on it +DETAIL: table foo column f1 depends on type cube +HINT: Use DROP ... CASCADE to drop the dependent objects too. +drop table foo; +drop extension cube; +-- list what's installed +\dT + List of data types + Schema | Name | Description +--------+------+------------- +(0 rows) + +\df + List of functions + Schema | Name | Result data type | Argument data types | Type +--------+------+------------------+---------------------+------ +(0 rows) + +\do + List of operators + Schema | Name | Left arg type | Right arg type | Result type | Description +--------+------+---------------+----------------+-------------+------------- +(0 rows) + +create schema c; +create extension cube with schema c; +-- list what's installed +\dT public.* + List of data types + Schema | Name | Description +--------+------+------------- +(0 rows) + +\df public.* + List of functions + Schema | Name | Result data type | Argument data types | Type +--------+------+------------------+---------------------+------ +(0 rows) + +\do public.* + List of operators + Schema | Name | Left arg type | Right arg type | Result type | Description +--------+------+---------------+----------------+-------------+------------- +(0 rows) + +\dT c.* + List of data types + Schema | Name | Description +--------+--------+--------------------------------------------------------------------------------------------- + c | c.cube | multi-dimensional cube '(FLOAT-1, FLOAT-2, ..., FLOAT-N), (FLOAT-1, FLOAT-2, ..., FLOAT-N)' +(1 row) + +\df c.* + List of functions + Schema | Name | Result data type | Argument data types | Type +--------+-------------------+------------------+--------------------------------------------+-------- + c | cube | c.cube | c.cube, double precision | normal + c | cube | c.cube | c.cube, double precision, double precision | normal + c | cube | c.cube | double precision | normal + c | cube | c.cube | double precision, double precision | normal + c | cube | c.cube | double precision[] | normal + c | cube | c.cube | double precision[], double precision[] | normal + c | cube_cmp | integer | c.cube, c.cube | normal + c | cube_contained | boolean | c.cube, c.cube | normal + c | cube_contains | boolean | c.cube, c.cube | normal + c | cube_dim | integer | c.cube | normal + c | cube_distance | double precision | c.cube, c.cube | normal + c | cube_enlarge | c.cube | c.cube, double precision, integer | normal + c | cube_eq | boolean | c.cube, c.cube | normal + c | cube_ge | boolean | c.cube, c.cube | normal + c | cube_gt | boolean | c.cube, c.cube | normal + c | cube_in | c.cube | cstring | normal + c | cube_inter | c.cube | c.cube, c.cube | normal + c | cube_is_point | boolean | c.cube | normal + c | cube_le | boolean | c.cube, c.cube | normal + c | cube_ll_coord | double precision | c.cube, integer | normal + c | cube_lt | boolean | c.cube, c.cube | normal + c | cube_ne | boolean | c.cube, c.cube | normal + c | cube_out | cstring | c.cube | normal + c | cube_overlap | boolean | c.cube, c.cube | normal + c | cube_size | double precision | c.cube | normal + c | cube_subset | c.cube | c.cube, integer[] | normal + c | cube_union | c.cube | c.cube, c.cube | normal + c | cube_ur_coord | double precision | c.cube, integer | normal + c | g_cube_compress | internal | internal | normal + c | g_cube_consistent | boolean | internal, c.cube, integer, oid, internal | normal + c | g_cube_decompress | internal | internal | normal + c | g_cube_penalty | internal | internal, internal, internal | normal + c | g_cube_picksplit | internal | internal, internal | normal + c | g_cube_same | internal | c.cube, c.cube, internal | normal + c | g_cube_union | c.cube | internal, internal | normal +(35 rows) + +\do c.* + List of operators + Schema | Name | Left arg type | Right arg type | Result type | Description +--------+------+---------------+----------------+-------------+-------------------------- + c | && | c.cube | c.cube | boolean | overlaps + c | < | c.cube | c.cube | boolean | lower than + c | <= | c.cube | c.cube | boolean | lower than or equal to + c | <> | c.cube | c.cube | boolean | different + c | <@ | c.cube | c.cube | boolean | contained in + c | = | c.cube | c.cube | boolean | same as + c | > | c.cube | c.cube | boolean | greater than + c | >= | c.cube | c.cube | boolean | greater than or equal to + c | @ | c.cube | c.cube | boolean | contains + c | @> | c.cube | c.cube | boolean | contains + c | ~ | c.cube | c.cube | boolean | contained in +(11 rows) + +create table foo (f1 c.cube, f2 int); +drop extension cube; -- fail, foo.f1 requires it +ERROR: cannot drop extension cube because other objects depend on it +DETAIL: table foo column f1 depends on type c.cube +HINT: Use DROP ... CASCADE to drop the dependent objects too. +drop schema c; -- fail, cube requires it +ERROR: cannot drop schema c because other objects depend on it +DETAIL: extension cube depends on schema c +table foo column f1 depends on type c.cube +HINT: Use DROP ... CASCADE to drop the dependent objects too. +drop extension cube cascade; +NOTICE: drop cascades to table foo column f1 +\d foo + Table "public.foo" + Column | Type | Modifiers +--------+---------+----------- + f2 | integer | + +-- list what's installed +\dT public.* + List of data types + Schema | Name | Description +--------+------+------------- +(0 rows) + +\df public.* + List of functions + Schema | Name | Result data type | Argument data types | Type +--------+------+------------------+---------------------+------ +(0 rows) + +\do public.* + List of operators + Schema | Name | Left arg type | Right arg type | Result type | Description +--------+------+---------------+----------------+-------------+------------- +(0 rows) + +\dT c.* + List of data types + Schema | Name | Description +--------+------+------------- +(0 rows) + +\df c.* + List of functions + Schema | Name | Result data type | Argument data types | Type +--------+------+------------------+---------------------+------ +(0 rows) + +\do c.* + List of operators + Schema | Name | Left arg type | Right arg type | Result type | Description +--------+------+---------------+----------------+-------------+------------- +(0 rows) + +drop schema c; diff --git a/contrib/earthdistance/sql/earthdistance.sql b/contrib/earthdistance/sql/earthdistance.sql index e494c350ce..23746f6fe9 100644 --- a/contrib/earthdistance/sql/earthdistance.sql +++ b/contrib/earthdistance/sql/earthdistance.sql @@ -1,7 +1,14 @@ -- --- Test earth distance functions +-- Test earthdistance extension +-- +-- In this file we also do some testing of extension create/drop scenarios. +-- That's really exercising the core database's dependency logic, so ideally +-- we'd do it in the core regression tests, but we can't for lack of suitable +-- guaranteed-available extensions. earthdistance is a good test case because +-- it has a dependency on the cube extension. -- +CREATE EXTENSION earthdistance; -- fail, must install cube first CREATE EXTENSION cube; CREATE EXTENSION earthdistance; @@ -291,3 +298,68 @@ SELECT is_point(ll_to_earth(-30,-90)); SELECT cube_dim(ll_to_earth(-30,-90)) <= 3; SELECT abs(cube_distance(ll_to_earth(-30,-90), '(0)'::cube) / earth() - 1) < '10e-12'::float8; + +-- +-- Now we are going to test extension create/drop scenarios. +-- + +-- list what's installed +\dT +\df +\do + +drop extension cube; -- fail, earthdistance requires it + +drop extension earthdistance; + +drop type cube; -- fail, extension cube requires it + +-- list what's installed +\dT +\df +\do + +create table foo (f1 cube, f2 int); + +drop extension cube; -- fail, foo.f1 requires it + +drop table foo; + +drop extension cube; + +-- list what's installed +\dT +\df +\do + +create schema c; + +create extension cube with schema c; + +-- list what's installed +\dT public.* +\df public.* +\do public.* +\dT c.* +\df c.* +\do c.* + +create table foo (f1 c.cube, f2 int); + +drop extension cube; -- fail, foo.f1 requires it + +drop schema c; -- fail, cube requires it + +drop extension cube cascade; + +\d foo + +-- list what's installed +\dT public.* +\df public.* +\do public.* +\dT c.* +\df c.* +\do c.* + +drop schema c; diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index c459c1e221..0526555ac3 100644 --- a/src/backend/catalog/dependency.c +++ b/src/backend/catalog/dependency.c @@ -98,6 +98,7 @@ typedef struct #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 */ @@ -190,6 +191,9 @@ static void add_exact_object_address_extra(const ObjectAddress *object, static bool object_address_present_add_flags(const ObjectAddress *object, int flags, ObjectAddresses *addrs); +static bool stack_address_present_add_flags(const ObjectAddress *object, + int flags, + ObjectAddressStack *stack); static void getRelationDescription(StringInfo buffer, Oid relid); static void getOpFamilyDescription(StringInfo buffer, Oid opfid); @@ -459,7 +463,6 @@ findDependentObjects(const ObjectAddress *object, ObjectAddress otherObject; ObjectAddressStack mystack; ObjectAddressExtra extra; - ObjectAddressStack *stackptr; /* * If the target object is already being visited in an outer recursion @@ -477,27 +480,8 @@ findDependentObjects(const ObjectAddress *object, * auto dependency, too, if we had to. However there are no known cases * where that would be necessary. */ - for (stackptr = stack; stackptr; stackptr = stackptr->next) - { - if (object->classId == stackptr->object->classId && - object->objectId == stackptr->object->objectId) - { - if (object->objectSubId == stackptr->object->objectSubId) - { - stackptr->flags |= flags; - return; - } - - /* - * Could visit column with whole table already on stack; this is - * the same case noted in object_address_present_add_flags(). - * (It's not clear this can really happen, but we might as well - * check.) - */ - if (stackptr->object->objectSubId == 0) - return; - } - } + if (stack_address_present_add_flags(object, flags, stack)) + return; /* * It's also possible that the target object has already been completely @@ -513,12 +497,13 @@ findDependentObjects(const ObjectAddress *object, /* * 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. + * (its "owner"), and/or be a member of an extension (also 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, @@ -567,7 +552,7 @@ findDependentObjects(const ObjectAddress *object, * 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 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. */ @@ -595,31 +580,31 @@ findDependentObjects(const ObjectAddress *object, /* * 2. When recursing from the other end of this dependency, - * it's okay to continue with the deletion. This holds when + * it's okay to continue with the deletion. This holds when * recursing from a whole object that includes the nominal - * other end as a component, too. + * other end as a component, too. Since there can be more + * than one "owning" object, we have to allow matches that + * are more than one level down in the stack. */ - if (stack->object->classId == otherObject.classId && - stack->object->objectId == otherObject.objectId && - (stack->object->objectSubId == otherObject.objectSubId || - stack->object->objectSubId == 0)) + if (stack_address_present_add_flags(&otherObject, 0, stack)) break; /* - * 3. When recursing from anyplace else, transform this - * deletion request into a delete of the other object. + * 3. Not all the owning objects have been visited, so + * transform this deletion request into a delete of this + * owning object. * * First, release caller's lock on this object and get - * deletion lock on the other object. (We must release + * deletion lock on the owning object. (We must release * caller's lock to avoid deadlock against a concurrent - * deletion of the other object.) + * deletion of the owning 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 + * 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). */ @@ -631,13 +616,18 @@ findDependentObjects(const ObjectAddress *object, } /* - * 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. + * 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, - flags, + DEPFLAG_REVERSE, stack, targetObjects, pendingObjects, @@ -2016,6 +2006,43 @@ object_address_present_add_flags(const ObjectAddress *object, return false; } +/* + * Similar to above, except we search an ObjectAddressStack. + */ +static bool +stack_address_present_add_flags(const ObjectAddress *object, + int flags, + ObjectAddressStack *stack) +{ + ObjectAddressStack *stackptr; + + for (stackptr = stack; stackptr; stackptr = stackptr->next) + { + const ObjectAddress *thisobj = stackptr->object; + + if (object->classId == thisobj->classId && + object->objectId == thisobj->objectId) + { + 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; +} + /* * Record multiple dependencies from an ObjectAddresses array, after first * removing any duplicates. -- 2.40.0