Thread: Memo on dropping practices
Now that the pg_depend mechanism is mostly in there, it is no longer
a good idea to delete things directly (for example, by calling
heap_drop_with_catalog or even just heap_delete'ing a catalog tuple).
The correct thing to do is to call performDeletion() with a parameter
specifying what it is you want to delete. Object deletion
commands should be implemented in two routines: an outer wrapper that
looks up the object, verifies permissions, and calls performDeletion,
and an inner routine that actually deletes the catalog entry (plus
any other directly-associated work). The inner routine is called from
performDeletion() after handling any dependency processing that might
be needed. A good example to look at is the way RemoveFunction()
has been split into RemoveFunction() and RemoveFunctionById().
The payoff for this seeming extra complexity is that we can get rid of
a lot of former hard-wired code in favor of letting dependencies do it.
For instance, heap_drop_with_catalog no longer does anything directly
about deleting indexes, constraints, or type tuples --- that's all
gotten rid of by dependency links when you do a DROP TABLE. Thus
heap.c is about 300 lines shorter than it used to be. We also have
much more control over whether to allow deletions of dependent objects.
For instance, you now get fairly sane behavior when you try to drop
the pg_type entry associated with a relation:
regression=# create table foo(f1 int);
CREATE TABLE
regression=# drop type foo;
ERROR: Cannot drop type foo because table foo requires it You may DROP the other object instead
I notice that Tatsuo recently committed DROP CONVERSION code that does
things the old way. I didn't try to change it, but as-is it will not
work to have any dependencies leading to or from conversions. I
recommend changing it so that it can participate in dependencies.
regards, tom lane
Tom Lane wrote: > Now that the pg_depend mechanism is mostly in there, it is no longer > a good idea to delete things directly (for example, by calling > heap_drop_with_catalog or even just heap_delete'ing a catalog tuple). > > The correct thing to do is to call performDeletion() with a parameter Should it be called performDrop rather than Deletion? > The payoff for this seeming extra complexity is that we can get rid of > a lot of former hard-wired code in favor of letting dependencies do it. > For instance, heap_drop_with_catalog no longer does anything directly > about deleting indexes, constraints, or type tuples --- that's all > gotten rid of by dependency links when you do a DROP TABLE. Thus > heap.c is about 300 lines shorter than it used to be. We also have > much more control over whether to allow deletions of dependent objects. > For instance, you now get fairly sane behavior when you try to drop > the pg_type entry associated with a relation: Yes, this code now allows lots of cleanups we weren't able to do before. TODO has:Dependency Checking===================* Add pg_depend table for dependency recording; use sysrelid, oid, depend_sysrelid,depend_oid, name* Auto-destroy sequence on DROP of table with SERIAL; perhaps a separate SERIAL type* HaveSERIAL generate non-colliding sequence names when we have auto-destruction* Prevent column dropping if column is usedby foreign key* Propagate column or table renaming to foreign key constraints* Automatically drop constraints/functionswhen object is dropped* Make constraints clearer in dump file* Make foreign keys easier to identify*Flush cached query plans when their underlying catalog data changes Which of these are done with the patch? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
> * Add pg_depend table for dependency recording; use sysrelid, oid, > depend_sysrelid, depend_oid, name > * Auto-destroy sequence on DROP of table with SERIAL; perhaps a separate > SERIAL type > * Have SERIAL generate non-colliding sequence names when we have > auto-destruction > * Prevent column dropping if column is used by foreign key > * Propagate column or table renaming to foreign key constraints > * Automatically drop constraints/functions when object is dropped > * Make constraints clearer in dump file > * Make foreign keys easier to identify > * Flush cached query plans when their underlying catalog data changes > > Which of these are done with the patch? Below is what I listed off as complete when submitting the patch. 'Make constraints clearer in dump file' is questionable. Foreign keys are, others not yet, but they need to be. # Add ALTER TABLE DROP non-CHECK CONSTRAINT # Allow psql \d to show foreign keys * Add pg_depend table for dependency recording; use sysrelid, oid, depend_sysrelid, depend_oid, name # Auto-destroy sequence on DROP of table with SERIAL # Prevent column dropping if column is used by foreign key # Automatically drop constraints/functions when object is dropped # Make constraints clearer in dump file # Make foreign keys easier to identify
Thanks, TODO updated. I split out "Make constraints clearer in dump file" into a foreign key version, which I marked as done, and a second version which I left as undone. Thanks. That's a heap of items completed. --------------------------------------------------------------------------- Rod Taylor wrote: > > * Add pg_depend table for dependency recording; use sysrelid, oid, > > depend_sysrelid, depend_oid, name > > * Auto-destroy sequence on DROP of table with SERIAL; perhaps a separate > > SERIAL type > > * Have SERIAL generate non-colliding sequence names when we have > > auto-destruction > > * Prevent column dropping if column is used by foreign key > > * Propagate column or table renaming to foreign key constraints > > * Automatically drop constraints/functions when object is dropped > > * Make constraints clearer in dump file > > * Make foreign keys easier to identify > > * Flush cached query plans when their underlying catalog data changes > > > > Which of these are done with the patch? > > Below is what I listed off as complete when submitting the patch. > > 'Make constraints clearer in dump file' is questionable. Foreign keys > are, others not yet, but they need to be. > > > # Add ALTER TABLE DROP non-CHECK CONSTRAINT > # Allow psql \d to show foreign keys > * Add pg_depend table for dependency recording; use sysrelid, oid, > depend_sysrelid, depend_oid, name > # Auto-destroy sequence on DROP of table with SERIAL > # Prevent column dropping if column is used by foreign key > # Automatically drop constraints/functions when object is dropped > # Make constraints clearer in dump file > # Make foreign keys easier to identify > > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
On Fri, 2002-07-12 at 15:17, Tom Lane wrote: > Now that the pg_depend mechanism is mostly in there, it is no longer > a good idea to delete things directly (for example, by calling > heap_drop_with_catalog or even just heap_delete'ing a catalog tuple). I noticed that SERIAL sequences aren't dropping with the application of the patch. Was this intentional? I know I didn't have a way of carrying sequence information across a dump (yet), but didn't think it would hurt to have.
Rod Taylor <rbt@zort.ca> writes:
>> Which of these are done with the patch?
> Below is what I listed off as complete when submitting the patch.
Note that I have not yet finished committing all of Rod's original
patch.
regards, tom lane
Rod Taylor <rbt@zort.ca> writes:
> I noticed that SERIAL sequences aren't dropping with the application of
> the patch.
> Was this intentional?
Yeah, the dependency isn't stored yet. I didn't like the way you did
that, and was trying to think of a better way...
More generally, a lot of dependencies that should be in place aren't
yet. I committed as soon as I had stable functionality that more or
less duplicated the former behavior; there's more patch still to review.
regards, tom lane
On Sat, 2002-07-13 at 10:27, Tom Lane wrote: > Rod Taylor <rbt@zort.ca> writes: > > I noticed that SERIAL sequences aren't dropping with the application of > > the patch. > > > Was this intentional? > > Yeah, the dependency isn't stored yet. I didn't like the way you did > that, and was trying to think of a better way... > > More generally, a lot of dependencies that should be in place aren't > yet. I committed as soon as I had stable functionality that more or > less duplicated the former behavior; there's more patch still to review. I eventually figured that out. I had only initially noticed a couple of missing items which would have been easy to miss.
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> The correct thing to do is to call performDeletion() with a parameter
> Should it be called performDrop rather than Deletion?
Well, if you want to rationalize the naming of these various routines:
I think DROP ought to be associated with the SQL-level commands.
performDeletion is the next level down (since it doesn't do any
permissions checks) and then there are the bottom-level deletion
routines for each object type (which do even less). It would make sense
to choose different verbs for each level. Right now, since I just split
RemoveFoo into two routines and called the second one RemoveFooById,
it's not very mnemonic at all. Perhaps:
DropFoo --- top level, corresponds to SQL DROP command
performSomething -- dependency controller
RemoveFoo --- bottom level deleter
Not sure what "performSomething" should be called, but I'd like to
think of a verb that's not either Drop or Remove. I'm not wedded to
Remove for the bottom level, either. Thoughts?
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> The correct thing to do is to call performDeletion() with a parameter > > > Should it be called performDrop rather than Deletion? > > Well, if you want to rationalize the naming of these various routines: > > I think DROP ought to be associated with the SQL-level commands. > performDeletion is the next level down (since it doesn't do any > permissions checks) and then there are the bottom-level deletion > routines for each object type (which do even less). It would make sense > to choose different verbs for each level. Right now, since I just split > RemoveFoo into two routines and called the second one RemoveFooById, > it's not very mnemonic at all. Perhaps: > > DropFoo --- top level, corresponds to SQL DROP command > > performSomething -- dependency controller > > RemoveFoo --- bottom level deleter > > Not sure what "performSomething" should be called, but I'd like to > think of a verb that's not either Drop or Remove. I'm not wedded to > Remove for the bottom level, either. Thoughts? How about: > DropFoo --- top level, corresponds to SQL DROP command > DropCascadeFoo --- dependency controller > RemoveFoo --- bottom level deleter Is that accurate for cascade? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> How about:
>> DropFoo --- top level, corresponds to SQL DROP command
>> DropCascadeFoo --- dependency controller
>> RemoveFoo --- bottom level deleter
There is only one dependency controller; it's not Foo anything.
And I don't want to call it by the same verb as either the top
or bottom levels...
regards, tom lane