rdelaage
/
postgresql
mirror of https://git.postgresql.org/git/postgresql.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Delete deleteWhatDependsOn() in favor of more performDeletion() flag bits. 

deleteWhatDependsOn() had grown an uncomfortably large number of
assumptions about what it's used for.  There are actually only two minor
differences between what it does and what a regular performDeletion() call
can do, so let's invent additional bits in performDeletion's existing flags
argument that specify those behaviors, and get rid of deleteWhatDependsOn()
as such.  (We'd probably have done it this way from the start, except that
performDeletion didn't originally have a flags argument, IIRC.)

Also, add a SKIP_EXTENSIONS flag bit that prevents ever recursing to an
extension, and use that when dropping temporary objects at session end.
This provides a more general solution to the problem addressed in a hacky
way in commit 08dd23cec: if an extension script creates temp objects and
forgets to remove them again, the whole extension went away when its
contained temp objects were deleted.  The previous solution only covered
temp relations, but this solves it for all object types.

These changes require minor additions in dependency.c to pass the flags
to subroutines that previously didn't get them, but it's still a net
savings of code, and it seems cleaner than before.

Having done this, revert the special-case code added in 08dd23cec that
prevented addition of pg_depend records for temp table extension
membership, because that caused its own oddities: dropping an extension
that had created such a table didn't automatically remove the table,
leading to a failure if the table had another dependency on the extension
(such as use of an extension data type), or to a duplicate-name failure if
you then tried to recreate the extension.  But we keep the part that
prevents the pg_temp_nnn schema from becoming an extension member; we never
want that to happen.  Add a regression test case covering these behaviors.

Although this fixes some arguable bugs, we've heard few field complaints,
and any such problems are easily worked around by explicitly dropping temp
objects at the end of extension scripts (which seems like good practice
anyway).  So I won't risk a back-patch.

Discussion: https://postgr.es/m/e51f4311-f483-4dd0-1ccc-abec3c405110@BlueTreble.com
This commit is contained in:
Tom Lane 2016-12-02 14:57:35 -05:00
parent 13df76a537
commit b3427dade1
12 changed files with 229 additions and 130 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
doc/src/sgml/extend.sgml
View File

@ -390,6 +390,15 @@
schema(s) its member objects are within.
</para>
<para>
If an extension's script creates any temporary objects (such as temp
tables), those objects are treated as extension members for the
remainder of the current session, but are automatically dropped at
session end, as any temporary object would be. This is an exception
to the rule that extension member objects cannot be dropped without
dropping the whole extension.
</para>
<sect2>
<title>Extension Files</title>
@ -803,7 +812,8 @@ SELECT pg_catalog.pg_extension_config_dump('my_config', 'WHERE NOT standard_entr
environment that <command>CREATE EXTENSION</> provides for installation
scripts: in particular, <varname>search_path</> is set up in the same
way, and any new objects created by the script are automatically added
to the extension.
to the extension. Also, if the script chooses to drop extension member
objects, they are automatically dissociated from the extension.
</para>
<para>

174
src/backend/catalog/dependency.c
View File

@ -168,6 +168,7 @@ static const Oid object_classes[] = {
static void findDependentObjects(const ObjectAddress *object,
int objflags,
int flags,
ObjectAddressStack *stack,
ObjectAddresses *targetObjects,
@ -175,7 +176,7 @@ static void findDependentObjects(const ObjectAddress *object,
Relation *depRel);
static void reportDependentObjects(const ObjectAddresses *targetObjects,
DropBehavior behavior,
int msglevel,
int flags,
const ObjectAddress *origObject);
static void deleteOneObject(const ObjectAddress *object,
Relation *depRel, int32 flags);
@ -237,11 +238,17 @@ deleteObjectsInList(ObjectAddresses *targetObjects, Relation *depRel,
}
/*
* Delete all the objects in the proper order.
* Delete all the objects in the proper order, except that if told to, we
* should skip the original object(s).
*/
for (i = 0; i < targetObjects->numrefs; i++)
{
ObjectAddress *thisobj = targetObjects->refs + i;
ObjectAddressExtra *thisextra = targetObjects->extras + i;
if ((flags & PERFORM_DELETION_SKIP_ORIGINAL) &&
(thisextra->flags & DEPFLAG_ORIGINAL))
continue;
deleteOneObject(thisobj, depRel, flags);
}
@ -255,16 +262,32 @@ deleteObjectsInList(ObjectAddresses *targetObjects, Relation *depRel,
* according to the dependency type.
*
* This is the outer control routine for all forms of DROP that drop objects
* that can participate in dependencies. Note that the next two routines
* are variants on the same theme; if you change anything here you'll likely
* need to fix them too.
* that can participate in dependencies. Note that performMultipleDeletions
* is a variant on the same theme; if you change anything here you'll likely
* need to fix that too.
*
* flags should include PERFORM_DELETION_INTERNAL when the drop operation is
* not the direct result of a user-initiated action. For example, when a
* temporary schema is cleaned out so that a new backend can use it, or when
* a column default is dropped as an intermediate step while adding a new one,
* that's an internal operation. On the other hand, when we drop something
* because the user issued a DROP statement against it, that's not internal.
* Bits in the flags argument can include:
*
* PERFORM_DELETION_INTERNAL: indicates that the drop operation is not the
* direct result of a user-initiated action. For example, when a temporary
* schema is cleaned out so that a new backend can use it, or when a column
* default is dropped as an intermediate step while adding a new one, that's
* an internal operation. On the other hand, when we drop something because
* the user issued a DROP statement against it, that's not internal. Currently
* this suppresses calling event triggers and making some permissions checks.
*
* PERFORM_DELETION_CONCURRENTLY: perform the drop concurrently. This does
* not currently work for anything except dropping indexes; don't set it for
* other object types or you may get strange results.
*
* PERFORM_DELETION_QUIETLY: reduce message level from NOTICE to DEBUG2.
*
* PERFORM_DELETION_SKIP_ORIGINAL: do not delete the specified object(s),
* but only what depends on it/them.
*
* PERFORM_DELETION_SKIP_EXTENSIONS: do not delete extensions, even when
* deleting objects that are part of an extension. This should generally
* be used only when dropping temporary objects.
*/
void
performDeletion(const ObjectAddress *object,
@ -293,6 +316,7 @@ performDeletion(const ObjectAddress *object,
findDependentObjects(object,
DEPFLAG_ORIGINAL,
flags,
NULL, /* empty stack */
targetObjects,
NULL, /* no pendingObjects */
@ -303,7 +327,7 @@ performDeletion(const ObjectAddress *object,
*/
reportDependentObjects(targetObjects,
behavior,
NOTICE,
flags,
object);
/* do the deed */
@ -364,6 +388,7 @@ performMultipleDeletions(const ObjectAddresses *objects,
findDependentObjects(thisobj,
DEPFLAG_ORIGINAL,
flags,
NULL, /* empty stack */
targetObjects,
objects,
@ -378,7 +403,7 @@ performMultipleDeletions(const ObjectAddresses *objects,
*/
reportDependentObjects(targetObjects,
behavior,
NOTICE,
flags,
(objects->numrefs == 1 ? objects->refs : NULL));
/* do the deed */
@ -390,88 +415,6 @@ performMultipleDeletions(const ObjectAddresses *objects,
heap_close(depRel, RowExclusiveLock);
}
/*
* deleteWhatDependsOn: attempt to drop everything that depends on the
* specified object, though not the object itself. Behavior is always
* CASCADE.
*
* This is currently used only to clean out the contents of a schema
* (namespace): the passed object is a namespace. We normally want this
* to be done silently, so there's an option to suppress NOTICE messages.
*
* Note we don't fire object drop event triggers here; it would be wrong to do
* so for the current only use of this function, but if more callers are added
* this might need to be reconsidered.
*/
void
deleteWhatDependsOn(const ObjectAddress *object,
bool showNotices)
{
Relation depRel;
ObjectAddresses *targetObjects;
int i;
/*
* We save some cycles by opening pg_depend just once and passing the
* Relation pointer down to all the recursive deletion steps.
*/
depRel = heap_open(DependRelationId, RowExclusiveLock);
/*
* Acquire deletion lock on the target object. (Ideally the caller has
* done this already, but many places are sloppy about it.)
*/
AcquireDeletionLock(object, 0);
/*
* Construct a list of objects to delete (ie, the given object plus
* everything directly or indirectly dependent on it).
*/
targetObjects = new_object_addresses();
findDependentObjects(object,
DEPFLAG_ORIGINAL,
NULL, /* empty stack */
targetObjects,
NULL, /* no pendingObjects */
&depRel);
/*
* Check if deletion is allowed, and report about cascaded deletes.
*/
reportDependentObjects(targetObjects,
DROP_CASCADE,
showNotices ? NOTICE : DEBUG2,
object);
/*
* Delete all the objects in the proper order, except we skip the original
* object.
*/
for (i = 0; i < targetObjects->numrefs; i++)
{
ObjectAddress *thisobj = targetObjects->refs + i;
ObjectAddressExtra *thisextra = targetObjects->extras + i;
if (thisextra->flags & DEPFLAG_ORIGINAL)
continue;
/*
* Since this function is currently only used to clean out temporary
* schemas, we pass PERFORM_DELETION_INTERNAL here, indicating that
* the operation is an automatic system operation rather than a user
* action. If, in the future, this function is used for other
* purposes, we might need to revisit this.
*/
deleteOneObject(thisobj, &depRel, PERFORM_DELETION_INTERNAL);
}
/* And clean up */
free_object_addresses(targetObjects);
heap_close(depRel, RowExclusiveLock);
}
/*
* findDependentObjects - find all objects that depend on 'object'
*
@ -492,16 +435,22 @@ deleteWhatDependsOn(const ObjectAddress *object,
* its sub-objects too.
*
* object: the object to add to targetObjects and find dependencies on
* flags: flags to be ORed into the object's targetObjects entry
* objflags: flags to be ORed into the object's targetObjects entry
* flags: PERFORM_DELETION_xxx flags for the deletion operation as a whole
* stack: list of objects being visited in current recursion; topmost item
* is the object that we recursed from (NULL for external callers)
* targetObjects: list of objects that are scheduled to be deleted
* pendingObjects: list of other objects slated for destruction, but
* not necessarily in targetObjects yet (can be NULL if none)
* *depRel: already opened pg_depend relation
*
* Note: objflags describes the reason for visiting this particular object
* at this time, and is not passed down when recursing. The flags argument
* is passed down, since it describes what we're doing overall.
*/
static void
findDependentObjects(const ObjectAddress *object,
int objflags,
int flags,
ObjectAddressStack *stack,
ObjectAddresses *targetObjects,
@ -518,8 +467,8 @@ findDependentObjects(const ObjectAddress *object,
/*
* If the target object is already being visited in an outer recursion
* level, just report the current flags back to that level and exit. This
* is needed to avoid infinite recursion in the face of circular
* level, just report the current objflags back to that level and exit.
* This is needed to avoid infinite recursion in the face of circular
* dependencies.
*
* The stack check alone would result in dependency loops being broken at
@ -532,19 +481,19 @@ findDependentObjects(const ObjectAddress *object,
* auto dependency, too, if we had to. However there are no known cases
* where that would be necessary.
*/
if (stack_address_present_add_flags(object, flags, stack))
if (stack_address_present_add_flags(object, objflags, stack))
return;
/*
* It's also possible that the target object has already been completely
* processed and put into targetObjects. If so, again we just add the
* specified flags to its entry and return.
* specified objflags to its entry and return.
*
* (Note: in these early-exit cases we could release the caller-taken
* lock, since the object is presumably now locked multiple times; but it
* seems not worth the cycles.)
*/
if (object_address_present_add_flags(object, flags, targetObjects))
if (object_address_present_add_flags(object, objflags, targetObjects))
return;
/*
@ -597,6 +546,15 @@ findDependentObjects(const ObjectAddress *object,
case DEPENDENCY_EXTENSION:
/*
* If told to, ignore EXTENSION dependencies altogether. This
* flag is normally used to prevent dropping extensions during
* temporary-object cleanup, even if a temp object was created
* during an extension script.
*/
if (flags & PERFORM_DELETION_SKIP_EXTENSIONS)
break;
/*
* If the other object is the extension currently being
* created/altered, ignore this dependency and continue with
@ -699,6 +657,7 @@ findDependentObjects(const ObjectAddress *object,
*/
findDependentObjects(&otherObject,
DEPFLAG_REVERSE,
flags,
stack,
targetObjects,
pendingObjects,
@ -729,7 +688,7 @@ findDependentObjects(const ObjectAddress *object,
* they have to be deleted before the current object.
*/
mystack.object = object; /* set up a new stack level */
mystack.flags = flags;
mystack.flags = objflags;
mystack.next = stack;
ScanKeyInit(&key[0],
@ -783,7 +742,7 @@ findDependentObjects(const ObjectAddress *object,
continue;
}
/* Recurse, passing flags indicating the dependency type */
/* Recurse, passing objflags indicating the dependency type */
switch (foundDep->deptype)
{
case DEPENDENCY_NORMAL:
@ -820,6 +779,7 @@ findDependentObjects(const ObjectAddress *object,
findDependentObjects(&otherObject,
subflags,
flags,
&mystack,
targetObjects,
pendingObjects,
@ -850,16 +810,17 @@ findDependentObjects(const ObjectAddress *object,
*
* targetObjects: list of objects that are scheduled to be deleted
* behavior: RESTRICT or CASCADE
* msglevel: elog level for non-error report messages
* flags: other flags for the deletion operation
* origObject: base object of deletion, or NULL if not available
* (the latter case occurs in DROP OWNED)
*/
static void
reportDependentObjects(const ObjectAddresses *targetObjects,
DropBehavior behavior,
int msglevel,
int flags,
const ObjectAddress *origObject)
{
int msglevel = (flags & PERFORM_DELETION_QUIETLY) ? DEBUG2 : NOTICE;
bool ok = true;
StringInfoData clientdetail;
StringInfoData logdetail;
@ -1140,8 +1101,7 @@ doDeletion(const ObjectAddress *object, int flags)
if (relKind == RELKIND_INDEX)
{
bool concurrent = ((flags & PERFORM_DELETION_CONCURRENTLY)
== PERFORM_DELETION_CONCURRENTLY);
bool concurrent = ((flags & PERFORM_DELETION_CONCURRENTLY) != 0);
Assert(object->objectSubId == 0);
index_drop(object->objectId, concurrent);

7
src/backend/catalog/heap.c
View File

@ -1285,10 +1285,6 @@ heap_create_with_catalog(const char *relname,
* should they have any ACL entries. The same applies for extension
* dependencies.
*
* If it's a temp table, we do not make it an extension member; this
* prevents the unintuitive result that deletion of the temp table at
* session end would make the whole extension go away.
*
* Also, skip this in bootstrap mode, since we don't make dependencies
* while bootstrapping.
*/
@ -1309,8 +1305,7 @@ heap_create_with_catalog(const char *relname,
recordDependencyOnOwner(RelationRelationId, relid, ownerid);
if (relpersistence != RELPERSISTENCE_TEMP)
recordDependencyOnCurrentExtension(&myself, false);
recordDependencyOnCurrentExtension(&myself, false);
if (reloftypeid)
{

11
src/backend/catalog/namespace.c
View File

@ -3872,14 +3872,19 @@ RemoveTempRelations(Oid tempNamespaceId)
/*
* We want to get rid of everything in the target namespace, but not the
* namespace itself (deleting it only to recreate it later would be a
* waste of cycles). We do this by finding everything that has a
* dependency on the namespace.
* waste of cycles). Hence, specify SKIP_ORIGINAL. It's also an INTERNAL
* deletion, and we want to not drop any extensions that might happen to
* own temp objects.
*/
object.classId = NamespaceRelationId;
object.objectId = tempNamespaceId;
object.objectSubId = 0;
deleteWhatDependsOn(&object, false);
performDeletion(&object, DROP_CASCADE,
PERFORM_DELETION_INTERNAL |
PERFORM_DELETION_QUIETLY |
PERFORM_DELETION_SKIP_ORIGINAL |
PERFORM_DELETION_SKIP_EXTENSIONS);
}
/*

5
src/backend/postmaster/autovacuum.c
View File

@ -2218,7 +2218,10 @@ do_autovacuum(void)
object.classId = RelationRelationId;
object.objectId = relid;
object.objectSubId = 0;
performDeletion(&object, DROP_CASCADE, PERFORM_DELETION_INTERNAL);
performDeletion(&object, DROP_CASCADE,
PERFORM_DELETION_INTERNAL |
PERFORM_DELETION_QUIETLY |
PERFORM_DELETION_SKIP_EXTENSIONS);
/*
* To commit the deletion, end current transaction and start a new

13
src/include/catalog/dependency.h
View File

@ -166,21 +166,22 @@ typedef enum ObjectClass
#define LAST_OCLASS OCLASS_TRANSFORM
/* flag bits for performDeletion/performMultipleDeletions: */
#define PERFORM_DELETION_INTERNAL 0x0001 /* internal action */
#define PERFORM_DELETION_CONCURRENTLY 0x0002 /* concurrent drop */
#define PERFORM_DELETION_QUIETLY 0x0004 /* suppress notices */
#define PERFORM_DELETION_SKIP_ORIGINAL 0x0008 /* keep original obj */
#define PERFORM_DELETION_SKIP_EXTENSIONS 0x0010 /* keep extensions */
/* in dependency.c */
#define PERFORM_DELETION_INTERNAL 0x0001
#define PERFORM_DELETION_CONCURRENTLY 0x0002
extern void performDeletion(const ObjectAddress *object,
DropBehavior behavior, int flags);
extern void performMultipleDeletions(const ObjectAddresses *objects,
DropBehavior behavior, int flags);
extern void deleteWhatDependsOn(const ObjectAddress *object,
bool showNotices);
extern void recordDependencyOnExpr(const ObjectAddress *depender,
Node *expr, List *rtable,
DependencyType behavior);

11
src/include/commands/extension.h
View File

@ -19,10 +19,13 @@
/*
* creating_extension is only true while running a CREATE EXTENSION command.
* It instructs recordDependencyOnCurrentExtension() to register a dependency
* on the current pg_extension object for each SQL object created by its
* installation script.
* creating_extension is only true while running a CREATE EXTENSION or ALTER
* EXTENSION UPDATE command. It instructs recordDependencyOnCurrentExtension()
* to register a dependency on the current pg_extension object for each SQL
* object created by an extension script. It also instructs performDeletion()
* to remove such dependencies without following them, so that extension
* scripts can drop member objects without having to explicitly dissociate
* them from the extension first.
*/
extern PGDLLIMPORT bool creating_extension;
extern Oid CurrentExtensionObject;

4
src/test/modules/test_extensions/Makefile
View File

@ -4,10 +4,10 @@ MODULE = test_extensions
PGFILEDESC = "test_extensions - regression testing for EXTENSION support"
EXTENSION = test_ext1 test_ext2 test_ext3 test_ext4 test_ext5 test_ext6 \
test_ext7 test_ext_cyclic1 test_ext_cyclic2
test_ext7 test_ext8 test_ext_cyclic1 test_ext_cyclic2
DATA = test_ext1--1.0.sql test_ext2--1.0.sql test_ext3--1.0.sql \
test_ext4--1.0.sql test_ext5--1.0.sql test_ext6--1.0.sql \
test_ext7--1.0.sql test_ext7--1.0--2.0.sql \
test_ext7--1.0.sql test_ext7--1.0--2.0.sql test_ext8--1.0.sql \
test_ext_cyclic1--1.0.sql test_ext_cyclic2--1.0.sql
REGRESS = test_extensions test_extdepend

58
src/test/modules/test_extensions/expected/test_extensions.out
View File

@ -63,3 +63,61 @@ Objects in extension "test_ext7"
table ext7_table2
(2 rows)
-- test handling of temp objects created by extensions
create extension test_ext8;
-- \dx+ would expose a variable pg_temp_nn schema name, so we can't use it here
select regexp_replace(pg_describe_object(classid, objid, objsubid),
'pg_temp_\d+', 'pg_temp', 'g') as "Object Description"
from pg_depend
where refclassid = 'pg_extension'::regclass and deptype = 'e' and
refobjid = (select oid from pg_extension where extname = 'test_ext8')
order by 1;
Object Description
-----------------------------------------
function ext8_even(posint)
function pg_temp.ext8_temp_even(posint)
table ext8_table1
table ext8_temp_table1
type posint
(5 rows)
-- Should be possible to drop and recreate this extension
drop extension test_ext8;
create extension test_ext8;
select regexp_replace(pg_describe_object(classid, objid, objsubid),
'pg_temp_\d+', 'pg_temp', 'g') as "Object Description"
from pg_depend
where refclassid = 'pg_extension'::regclass and deptype = 'e' and
refobjid = (select oid from pg_extension where extname = 'test_ext8')
order by 1;
Object Description
-----------------------------------------
function ext8_even(posint)
function pg_temp.ext8_temp_even(posint)
table ext8_table1
table ext8_temp_table1
type posint
(5 rows)
-- here we want to start a new session and wait till old one is gone
select pg_backend_pid() as oldpid \gset
\c -
do 'declare c int = 0;
begin
while (select count(*) from pg_stat_activity where pid = '
:'oldpid'
') > 0 loop c := c + 1; end loop;
raise log ''test_extensions looped % times'', c;
end';
-- extension should now contain no temp objects
\dx+ test_ext8
Objects in extension "test_ext8"
Object Description
----------------------------
function ext8_even(posint)
table ext8_table1
type posint
(3 rows)
-- dropping it should still work
drop extension test_ext8;

39
src/test/modules/test_extensions/sql/test_extensions.sql
View File

@ -25,3 +25,42 @@ create extension test_ext7;
\dx+ test_ext7
alter extension test_ext7 update to '2.0';
\dx+ test_ext7
-- test handling of temp objects created by extensions
create extension test_ext8;
-- \dx+ would expose a variable pg_temp_nn schema name, so we can't use it here
select regexp_replace(pg_describe_object(classid, objid, objsubid),
'pg_temp_\d+', 'pg_temp', 'g') as "Object Description"
from pg_depend
where refclassid = 'pg_extension'::regclass and deptype = 'e' and
refobjid = (select oid from pg_extension where extname = 'test_ext8')
order by 1;
-- Should be possible to drop and recreate this extension
drop extension test_ext8;
create extension test_ext8;
select regexp_replace(pg_describe_object(classid, objid, objsubid),
'pg_temp_\d+', 'pg_temp', 'g') as "Object Description"
from pg_depend
where refclassid = 'pg_extension'::regclass and deptype = 'e' and
refobjid = (select oid from pg_extension where extname = 'test_ext8')
order by 1;
-- here we want to start a new session and wait till old one is gone
select pg_backend_pid() as oldpid \gset
\c -
do 'declare c int = 0;
begin
while (select count(*) from pg_stat_activity where pid = '
:'oldpid'
') > 0 loop c := c + 1; end loop;
raise log ''test_extensions looped % times'', c;
end';
-- extension should now contain no temp objects
\dx+ test_ext8
-- dropping it should still work
drop extension test_ext8;

21
src/test/modules/test_extensions/test_ext8--1.0.sql Normal file
View File

@ -0,0 +1,21 @@
/* src/test/modules/test_extensions/test_ext8--1.0.sql */
-- complain if script is sourced in psql, rather than via CREATE EXTENSION
\echo Use "CREATE EXTENSION test_ext8" to load this file. \quit
-- create some random data type
create domain posint as int check (value > 0);
-- use it in regular and temporary tables and functions
create table ext8_table1 (f1 posint);
create temp table ext8_temp_table1 (f1 posint);
create function ext8_even (posint) returns bool as
'select ($1 % 2) = 0' language sql;
create function pg_temp.ext8_temp_even (posint) returns bool as
'select ($1 % 2) = 0' language sql;
-- we intentionally don't drop the temp objects before exiting

4
src/test/modules/test_extensions/test_ext8.control Normal file
View File

@ -0,0 +1,4 @@
comment = 'Test extension 8'
default_version = '1.0'
schema = 'public'
relocatable = false