Re: Tentative patch for making DROP put dependency info in DETAIL

On Thu, Jun 12, 2008 at 11:58 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 ? :)