Re: [PATCHES] Tentative patch for making DROP put dependency info in DETAIL
Tom Lane wrote: multi-object DROP IF NOT EXISTS would fail to perform as expected. Surely this would be a noop :-) cheers andrew -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Tentative patch for making DROP put dependency info in DETAIL
"Alex Hunsaker" <[EMAIL PROTECTED]> 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Tentative patch for making DROP put dependency info in DETAIL
On Thu, Jun 12, 2008 at 5:35 PM, Tom Lane <[EMAIL PROTECTED]> 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(-) refactor_dropstmt.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Tentative patch for making DROP put dependency info in DETAIL
On Thu, Jun 12, 2008 at 5:35 PM, Tom Lane <[EMAIL PROTECTED]> wrote: > "Alex Hunsaker" <[EMAIL PROTECTED]> 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? -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Tentative patch for making DROP put dependency info in DETAIL
"Alex Hunsaker" <[EMAIL PROTECTED]> 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Tentative patch for making DROP put dependency info in DETAIL
On Thu, Jun 12, 2008 at 11:58 AM, Tom Lane <[EMAIL PROTECTED]> 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 ? :) -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Tentative patch for making DROP put dependency info in DETAIL
Alvaro Herrera <[EMAIL PROTECTED]> 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Tentative patch for making DROP put dependency info in DETAIL
On Thu, Jun 12, 2008 at 11:35 AM, Alvaro Herrera <[EMAIL PROTECTED]> 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. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Tentative patch for making DROP put dependency info in DETAIL
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 Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Tentative patch for making DROP put dependency info in DETAIL
On Wed, Jun 11, 2008 at 4:07 PM, Tom Lane <[EMAIL PROTECTED]> 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 dropStmt_mutlidelete.patch Description: Binary data -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Tentative patch for making DROP put dependency info in DETAIL
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 Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Tentative patch for making DROP put dependency info in DETAIL
Alvaro Herrera <[EMAIL PROTECTED]> 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 -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Tentative patch for making DROP put dependency info in DETAIL
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 Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
[PATCHES] 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 binLODlX7W7vf.bin Description: dependency-notices-1.patch.gz -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches