Thread: Tentative patch for making DROP put dependency info in DETAIL
There was some discussion a few days ago about making dependency.c emit
dependency reports in the same style that pg_shdepend.c does, viz
a lot of DETAIL lines on a single message instead of separate NOTICE
messages. Attached is a tentative patch that does that. See the
regression-test diffs for samples of what the output looks like.
I'm not entirely sure whether I like the results better than what
we have. Opinions anyone? There are some cases where it seems
clearly better, eg the sequence.out changes, but in a lot of others
it doesn't seem much better.
One particular case of interest is in truncate.out, where the
table-at-a-time implementation of DROP TABLE is clearly exposed
by the fact that you get multiple NOTICEs. I wonder if it would
be worth refactoring the code so that a multiple-object DROP is
implemented via performMultipleDeletions(). This would have more
than just cosmetic advantages: it would no longer matter what
order you listed the tables in. But the refactoring required looks
bigger and more tedious than I want to tackle right now.
I also want to note that we've historically had the code report
auto-cascade drops as DEBUG2 messages. I tried to merge those reports
into the main one but it was a complete mess :-( because the client and
server-log messages might have different numbers of entries depending on
their log-level settings. Almost the first case I tried favored me with
NOTICE: drop cascades to 0 other object(s)
DETAIL:
reported to the client (with the server log of course saying something
different). So I gave up and left that behavior separate.
Comments? Should we do this, or leave things alone?
regards, tom lane
Attachment
Tom Lane wrote: > One particular case of interest is in truncate.out, where the > table-at-a-time implementation of DROP TABLE is clearly exposed > by the fact that you get multiple NOTICEs. I wonder if it would > be worth refactoring the code so that a multiple-object DROP is > implemented via performMultipleDeletions(). This would have more > than just cosmetic advantages: it would no longer matter what > order you listed the tables in. But the refactoring required looks > bigger and more tedious than I want to tackle right now. Hmm, this is a bit ugly. I'd vote for doing the refactoring. However, I'd say you should commit the patch you currently have and let one of the younger hackers fix that problem -- it looks like an good beginner project. > I also want to note that we've historically had the code report > auto-cascade drops as DEBUG2 messages. I tried to merge those reports > into the main one but it was a complete mess :-( because the client and > server-log messages might have different numbers of entries depending on > their log-level settings. Almost the first case I tried favored me with > NOTICE: drop cascades to 0 other object(s) > DETAIL: > reported to the client (with the server log of course saying something > different). So I gave up and left that behavior separate. Huh, annoying. Agreed with leaving that alone. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane wrote:
>> ... I wonder if it would
>> be worth refactoring the code so that a multiple-object DROP is
>> implemented via performMultipleDeletions(). This would have more
>> than just cosmetic advantages: it would no longer matter what
>> order you listed the tables in. But the refactoring required looks
>> bigger and more tedious than I want to tackle right now.
> Hmm, this is a bit ugly. I'd vote for doing the refactoring. However,
> I'd say you should commit the patch you currently have and let one of
> the younger hackers fix that problem -- it looks like an good beginner
> project.
Agreed --- I committed what I had, anyone want to volunteer for
refactoring the execution of DropStmt?
After looking again, I think that this is not technically very
difficult, but coming up with something that looks tasteful to everyone
might be tricky. In particular I didn't see a nice way to do it without
using struct ObjectAddress in a bunch of header files that don't
currently include dependency.h. A possible response to that is to move
ObjectAddress into postgres.h, but that seems a bit ugly too.
regards, tom lane
Tom Lane wrote: > After looking again, I think that this is not technically very > difficult, but coming up with something that looks tasteful to everyone > might be tricky. In particular I didn't see a nice way to do it without > using struct ObjectAddress in a bunch of header files that don't > currently include dependency.h. A possible response to that is to move > ObjectAddress into postgres.h, but that seems a bit ugly too. Ugh. I thought about having a new header, but that seems overkill. -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
On Wed, Jun 11, 2008 at 4:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Agreed --- I committed what I had, anyone want to volunteer for > refactoring the execution of DropStmt? Sure! see the attached patch... > After looking again, I think that this is not technically very > difficult, but coming up with something that looks tasteful to everyone > might be tricky. In particular I didn't see a nice way to do it without > using struct ObjectAddress in a bunch of header files that don't > currently include dependency.h. A possible response to that is to move > ObjectAddress into postgres.h, but that seems a bit ugly too. Ok I'm obviously missing something important... Why not Just make the various Remove* functions take a list? I'm not proposing this patch for actual submission, more of a would this work? If I'm not missing something glaring obvious Ill go ahead and make the rest of the Remove things behave the same way
Attachment
Alex Hunsaker escribió: > Ok I'm obviously missing something important... Why not Just make the > various Remove* functions take a list? > > I'm not proposing this patch for actual submission, more of a would this work? > If I'm not missing something glaring obvious Ill go ahead and make the > rest of the Remove things behave the same way I don't think there's anything wrong with that in principle. However, does your patch actually work? The changes in expected/ is unexpected, I think. -- Alvaro Herrera http://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Thu, Jun 12, 2008 at 11:35 AM, Alvaro Herrera <alvherre@commandprompt.com> wrote: > I don't think there's anything wrong with that in principle. However, > does your patch actually work? The changes in expected/ is unexpected, > I think. Yeah I thought they looked a bit odd at first to. I thought it would just get rid of the duplicate NOTICES's. On closer look they don't NOITCE anymore because all the tables are listed in the drop. Here is an example: # with all them in in drop table create table test (a int primary key); create table test_a (a int references test); create table test_b (a int references test); drop table test, test_a, test_b cascade; DROP TABLE # now without test_b create table test (a int primary key); create table test_a (a int references test); create table test_b (a int references test); drop table test, test_a cascade; NOTICE: drop cascades to constraint test_b_a_fkey on table test_b DROP TABLE In fact you don't even need the cascade anymore if you specify all the dependent tables. So that certainly *seems* right to me.
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Alex Hunsaker escribi�:
>> I'm not proposing this patch for actual submission, more of a would this work?
>> If I'm not missing something glaring obvious Ill go ahead and make the
>> rest of the Remove things behave the same way
> I don't think there's anything wrong with that in principle. However,
> does your patch actually work? The changes in expected/ is unexpected,
> I think.
No, they look right --- we're losing gripes about earlier tables
cascading to subobjects of later tables, which is exactly what we want.
I don't really like the patch though; it seems kind of a brute force
solution. You've got ProcessUtility iterating through a list of objects
and doing a little bit of work on each one, and then making a new list
that RemoveRelation (which is now misnamed) again iterates through and
does a little bit of work on each one, and then passes that off *again*.
There's no principled division of labor at work there; in particular
it's far from obvious where things get locked. You've also managed
to embed an assumption not previously present, that all the objects in
the list are of exactly the same type.
I think it would be better if the DropStmt loop were responsible
for constructing a list of ObjectAddresses that it handed off directly
to performMultipleDeletions. This is why I was imagining replacing
RemoveRelation et al with something that passed back an ObjectAddress,
and getting worried about introducing references to ObjectAddress into
all those header files. Another possibility would be to get rid of
RemoveRelation et al altogether and embed that code right into the
DropStmt processing (though at this point we'd need to split it out
of ProcessUtility; it's already a bit large for where it is). That
didn't seem tremendously clean either, though it would at least have
the virtue of improving consistency about where privilege checks etc
get done.
regards, tom lane
On Thu, Jun 12, 2008 at 11:58 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I don't really like the patch though; it seems kind of a brute force > solution. You've got ProcessUtility iterating through a list of objects > and doing a little bit of work on each one, and then making a new list > that RemoveRelation (which is now misnamed) again iterates through and > does a little bit of work on each one, and then passes that off *again*. > There's no principled division of labor at work there; in particular > it's far from obvious where things get locked. You've also managed > to embed an assumption not previously present, that all the objects in > the list are of exactly the same type. Yep, I thought about doing the reverse. Namely Just passing the DropStmt to RemoveRelation(s). But then all the permission check functions are in utility.c. Splitting those out seemed to be about the same as splitting out all the ObjectAddress stuff... Plus with the potential ugliness I thought maybe this (as it is in the patch) way while still ugly, might be the less of two uglies :) And besides it was brain dead simple... > I think it would be better if the DropStmt loop were responsible > for constructing a list of ObjectAddresses that it handed off directly > to performMultipleDeletions. This is why I was imagining replacing > RemoveRelation et al with something that passed back an ObjectAddress, > and getting worried about introducing references to ObjectAddress into > all those header files. Another possibility would be to get rid of > RemoveRelation et al altogether and embed that code right into the > DropStmt processing (though at this point we'd need to split it out > of ProcessUtility; it's already a bit large for where it is). That > didn't seem tremendously clean either, though it would at least have > the virtue of improving consistency about where privilege checks etc > get done. > It seems strange to have _not_ have the permission checks in RemoveRelation IMHO. Granted utility.c is the only thing that calls it... It seems equally strange to (re)move RemoveRelation and friends into utility.c. But really if some other user besides utility.c was going to use them wouldn't they want the permission checks? So my vote is to just move them into utility.c maybe even make them static (until someone else needs them obviosly). Make them return an ObjectAddress (so they wont be called RemoveType, but getTypeAddress or something) and be done with it. Thoughts? Unless you thought of a cleaner way ? :)
"Alex Hunsaker" <badalex@gmail.com> writes:
> Yep, I thought about doing the reverse. Namely Just passing the
> DropStmt to RemoveRelation(s). But then all the permission check
> functions are in utility.c. Splitting those out seemed to be about
> the same as splitting out all the ObjectAddress stuff...
Well, that might actually be a good approach: try to get ProcessUtility
back down to being just a dispatch switch. It's pretty much of a wart
that we're doing any permissions checking in utility.c at all. Possibly
those functions should be moved to aclchk.c and then used from
RemoveRelation(s) and friends, which would stay where they are but
change API.
I think the fundamental tension here is between two theories of
organizing the code: we've got the notion of "collect operations
on an object type together", which leads to eg putting
RemoveTSConfiguration in tsearchcmds.c, as against "collect similar
operations together", which leads to wanting all the DROP operations
in the same module.
In the abstract it's not too easy to choose between these, but I think
you'll probably find that the first one works better here, mainly
because each of those object-type modules knows how to work with a
particular system catalog. A DROP module is going to find itself
importing all the catalog headers plus probably utility routines from
all over. So that's leading me to lean towards keeping RemoveRelation
et al where they are and distributing the work currently done in
ProcessUtility out to them. This sounds duplicative, but about all that
really is there to duplicate is a foreach loop, which you're going to
need anyway if the routines are to handle multiple objects.
regards, tom lane
On Thu, Jun 12, 2008 at 5:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Alex Hunsaker" <badalex@gmail.com> writes: >> Yep, I thought about doing the reverse. Namely Just passing the >> DropStmt to RemoveRelation(s). But then all the permission check >> functions are in utility.c. Splitting those out seemed to be about >> the same as splitting out all the ObjectAddress stuff... > > Well, that might actually be a good approach: try to get ProcessUtility > back down to being just a dispatch switch. It's pretty much of a wart > that we're doing any permissions checking in utility.c at all. Possibly > those functions should be moved to aclchk.c and then used from > RemoveRelation(s) and friends, which would stay where they are but > change API. Ok Ill work up a patch. Whats that saying about sticking with your first instinct?
On Thu, Jun 12, 2008 at 5:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > So that's leading me to lean towards keeping RemoveRelation > et al where they are and distributing the work currently done in > ProcessUtility out to them. This sounds duplicative, but about all that > really is there to duplicate is a foreach loop, which you're going to > need anyway if the routines are to handle multiple objects. Ok Here it is: -Moves CheckDropPermissions and friends from utility.c to aclchk.c (pg_drop_permission_check) -Makes all the Remove* functions take a DropStmt *, they each do their own foreach() loop and permission checks -removed RemoveView and RemoveIndex because they were exactly the same as RemoveRelation -added an "s" to the end of the Remove* functions to denote they may remove more than one (i.e. RemoveRelations) -consolidated RemoveType and RemoveDomain into a common function (static void removeHelper()) -made performMultipleDeletions when we only have one item we are deleting log the same way (with the object name) src/backend/catalog/aclchk.c | 154 +++++++++++++++ src/backend/catalog/dependency.c | 9 +- src/backend/catalog/pg_conversion.c | 54 ++++--- src/backend/commands/conversioncmds.c | 45 +++-- src/backend/commands/indexcmds.c | 27 --- src/backend/commands/schemacmds.c | 91 +++++---- src/backend/commands/tablecmds.c | 66 ++++++- src/backend/commands/tsearchcmds.c | 290 +++++++++++++++++------------ src/backend/commands/typecmds.c | 189 ++++++++----------- src/backend/commands/view.c | 23 --- src/backend/tcop/utility.c | 288 +++++------------------------ src/include/catalog/pg_conversion_fn.h | 2 +- src/include/commands/conversioncmds.h | 3 +- src/include/commands/defrem.h | 14 +- src/include/commands/schemacmds.h | 2 +- src/include/commands/tablecmds.h | 2 +- src/include/commands/typecmds.h | 4 +- src/include/commands/view.h | 2 +- src/include/utils/acl.h | 1 + src/test/regress/expected/foreign_key.out | 11 - src/test/regress/expected/truncate.out | 6 - 21 files changed, 645 insertions(+), 638 deletions(-)
Attachment
"Alex Hunsaker" <badalex@gmail.com> writes:
> Ok Here it is:
> -Moves CheckDropPermissions and friends from utility.c to aclchk.c
> (pg_drop_permission_check)
> -Makes all the Remove* functions take a DropStmt *, they each do their
> own foreach() loop and permission checks
Applied with revisions. I had suggested moving stuff into aclchk.c on
the assumption that it needed to be called from more than one place.
But after you got rid of the separate RemoveIndex and RemoveView
functions (which was a good idea), there was only one call site for
those functions, so I just folded them into tablecmds.c; and in fact
integrated CheckDropPermissions right into RemoveRelations so it looked
more like all the other DropStmt functions. Also,
RemoveTypes/RemoveDomains might as well be integrated completely since
we're doing relations that way. I also chose to clean up the conversion
dropping stuff, since there didn't seem to be any point in the
separation between ConversionDrop and DropConversionCommand.
The only actual bug I found was that you'd used "break" where you should
have used "continue" for a non-existent object in each routine, so a
multi-object DROP IF NOT EXISTS would fail to perform as expected.
regards, tom lane
Tom Lane wrote: > multi-object DROP IF NOT EXISTS would fail to perform as expected. > > > Surely this would be a noop :-) cheers andrew