DROP OWNED BY doesn't work
So I was fooling with making serial sequences be auto rather than internal
dependencies of their columns, and the regression tests blew up on me:
*** ./expected/dependency.out Mon Nov 21 07:49:33 2005
--- ./results/dependency.out Sat Aug 19 17:46:55 2006
***************
*** 109,113 ****
--- 109,117 ----
DROP USER regression_user2;
ERROR: role "regression_user2" cannot be dropped because some objects depend on it
DROP OWNED BY regression_user2, regression_user0;
+ NOTICE: default for table deptest column a depends on sequence deptest_a_seq
+ ERROR: cannot drop sequence deptest_a_seq because other objects depend on it
DROP USER regression_user2;
+ ERROR: role "regression_user2" cannot be dropped because some objects depend on it
DROP USER regression_user0;
+ ERROR: role "regression_user0" cannot be dropped because some objects depend on itOn investigation I find that DROP OWNED BY tries to drop objects in the
more-or-less-random order that they appear in pg_shdepend. This doesn't
work, at least not without CASCADE. I think people should be able to
assume that DROP OWNED BY RESTRICT will drop all the target users'
objects so long as there are not objects owned by other users that
depend on them.
There's a hack in there now that tries to special-case this for INTERNAL
dependencies, but I think it's misguided. You can run into the problem
without any internal or auto dependencies:
regression=# create user foo;
CREATE ROLE
regression=# \c - foo
You are now connected to database "regression" as user "foo".
regression=> create table tt1 (f1 int);
CREATE TABLE
regression=> create table tt2 (f1 int primary key);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "tt2_pkey" for table "tt2"
CREATE TABLE
regression=> alter table tt1 add foreign key (f1) references tt2;
ALTER TABLE
regression=> drop owned by foo;
NOTICE: constraint tt1_f1_fkey on table tt1 depends on table tt2
ERROR: cannot drop table tt2 because other objects depend on it
HINT: Use DROP ... CASCADE to drop the dependent objects too.
regression=>
I think a correct solution probably requires making a list of all
objects to delete by scanning pg_shdepend and then starting to
delete 'em, using the list as "oktodelete" context similar to the
way that dependency.c handles auto/internal objects.
regards, tom lane
Tom Lane wrote:
I think a correct solution probably requires making a list of all
objects to delete by scanning pg_shdepend and then starting to
delete 'em, using the list as "oktodelete" context similar to the
way that dependency.c handles auto/internal objects.
Hum. I'll take a look at doing that.
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Tom Lane wrote:
I think a correct solution probably requires making a list of all
objects to delete by scanning pg_shdepend and then starting to
delete 'em, using the list as "oktodelete" context similar to the
way that dependency.c handles auto/internal objects.
What I'm considering is this: scan pg_shdepend looking for objects to
delete, and save them into a list; but each time we find one, we also
find objects that depend on it. Those dependent objects should be
ignored; but we should also remove from the list of objects to delete,
any dependent object that we added in a previous iteration of the
pg_shdepend scan. A subtlety is that if we see that an object from the
scan is in the ignored list, we need not find dependent objects nor add
it to the to-delete list.
In pseudo-code it would be something like this:
ObjectAddresses objects-to-delete = empty
ObjectAddresses objects-ignored = empty
scan pg_shdepend for objects depending on the user;
for (object) in scan
if (object is not in objects-ignored)
ObjectAddresses dependent-objects = empty
add object to objects-to-delete
dependent-objects = findAutoDeletableObjects(object)
add dependent-objects to objects-ignored
for (object2) in objects-to-delete
if (object2 is in objects-ignored)
remove object2 from objects-to-delete
add object2 fo objects-ignored
for (object) in objects-to-delete
performDeletion(object)
This has a nasty looking O(n^2) behavior, but I don't see anything
better.
Comments?
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera <alvherre@commandprompt.com> writes:
What I'm considering is this: scan pg_shdepend looking for objects to
delete, and save them into a list; but each time we find one, we also
find objects that depend on it. Those dependent objects should be
ignored; but we should also remove from the list of objects to delete,
any dependent object that we added in a previous iteration of the
pg_shdepend scan.
Um ... but this doesn't seem to acknowledge the idea that we should
distinguish whether those dependent objects are owned by the user or
not.
What I was thinking of was
find all target objects using pg_shdepend, make an
ObjectAddresses list of them
for (each item of list)
{
if (already deleted)
continue;
if (implicit dependency of some later list item)
continue;
pass item to performDeletion, with entire list
as oktodelete context
}
This will require some cooperation from dependency.c: the standard
performDeletion entry point doesn't let you pass in an oktodelete list,
and it would be nice if "performDeletionWithList" or whatever we
call it provided some feedback about what it'd deleted so that the
bookkeeping effort implicit in "if (already deleted)" could be
minimized.
This has a nasty looking O(n^2) behavior, but I don't see anything
better.
In my formulation the O(N^2) behaviors are limited to searching the
oktodelete list, which at least is a pretty tight inner loop. The
dependency.c code was not designed with the idea that oktodelete would
ever get really large ... perhaps later we could revisit that data
structure, but for now I think this'll be good enough.
regards, tom lane
Tom Lane wrote:
What I was thinking of was
find all target objects using pg_shdepend, make an
ObjectAddresses list of themfor (each item of list)
{
if (already deleted)
continue;
if (implicit dependency of some later list item)
continue;
pass item to performDeletion, with entire list
as oktodelete context
}
Ok, this patch implements more or less this idea; except that instead of
checking the list of implicit dependencies for later objects, we iterate
twice on the list of objects to delete, and create a list of implicit
dependencies on all of them the first time, and delete those not on that
list the second time.
Regression tests pass. This patch makes the following example work as
expected. Note that the first "drop owned" is stopped by the FK on tt3;
by adding the cascade, it drops the foreign key.
Let me know if it fixes your problem, and I'll commit it early tomorrow.
(Or you can do it if you're in a hurry ...)
create user foo;
create user bar;
\c - bar
create table tt3 (f1 int);
\c - foo
create table tt1 (f1 int);
create table tt2 (f1 int primary key);
alter table tt1 add foreign key (f1) references tt2;
grant references on table tt2 to bar;
\c - bar
alter table tt3 add foreign key (f1) references tt2;
\c - foo
drop owned by foo;
drop owned by foo cascade;
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachments:
drop-owned-bug.patchtext/plain; charset=us-asciiDownload+258-123
Alvaro Herrera <alvherre@commandprompt.com> writes:
Ok, this patch implements more or less this idea; except that instead of
checking the list of implicit dependencies for later objects, we iterate
twice on the list of objects to delete, and create a list of implicit
dependencies on all of them the first time, and delete those not on that
list the second time.
In the light of morning it occurs to me that probably the best code
factoring for this is for dependency.c to export a
performMultipleDeletions() function that takes an objectAddresses list
of targets and does all the dependency-chasing internally. This would
reduce the amount of cruft that has to be exported from dependency.c.
Other than that, the patch looks plausible in a very fast scan.
regards, tom lane
Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Ok, this patch implements more or less this idea; except that instead of
checking the list of implicit dependencies for later objects, we iterate
twice on the list of objects to delete, and create a list of implicit
dependencies on all of them the first time, and delete those not on that
list the second time.In the light of morning it occurs to me that probably the best code
factoring for this is for dependency.c to export a
performMultipleDeletions() function that takes an objectAddresses list
of targets and does all the dependency-chasing internally. This would
reduce the amount of cruft that has to be exported from dependency.c.
Sounds a pretty obvious move. This would be it then. Please note the
changes to the ObjectAddresses stuff, mainly to not move the definition
of struct ObjectAddresses to dependency.h but instead move only the
typedef.
--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.
Attachments:
drop-owned-bug-2.patchtext/plain; charset=us-asciiDownload+284-107
Alvaro Herrera <alvherre@commandprompt.com> writes:
Sounds a pretty obvious move. This would be it then. Please note the
changes to the ObjectAddresses stuff, mainly to not move the definition
of struct ObjectAddresses to dependency.h but instead move only the
typedef.
Looks pretty reasonable, except if you're going to do the latter I'd
suggest just having new_object_addresses() and free_object_addresses()
and getting rid of the init/term API for having the structs on the
stack. The latter is saving one palloc/pfree per deletion cycle, which
is a pretty silly micro-optimization really given everything else that's
going to happen to perform the delete. It was OK when there wasn't any
reason to do otherwise, but I can't see maintaining it in parallel with a
version that does the extra palloc/pfree, and especially not adding a
bool to the struct to support having both ...
Please fix that bit and apply.
regards, tom lane
I found one other problem in this area, which was that REASSIGN OWNED
didn't work real well either after I changed serial sequences'
dependency type to AUTO. What I did about it was to make
shdepReassignOwned call ATExecChangeOwner with recursing = true,
which suppresses all those tedious error checks ;-). This means we
don't need objectIsInternalDependency() at all anymore: just barrel
ahead and try to change owner on everything.
regards, tom lane