Skip site navigation (1) Skip section navigation (2)

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

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Alvaro Herrera <alvherre(at)commandprompt(dot)com>
Cc: Alex Hunsaker <badalex(at)gmail(dot)com>, pgsql-patches(at)postgresql(dot)org
Subject: Re: Tentative patch for making DROP put dependency info in DETAIL
Date: 2008-06-12 17:58:47
Message-ID: 11924.1213293527@sss.pgh.pa.us (view raw or flat)
Thread:
Lists: pgsql-patches
Alvaro Herrera <alvherre(at)commandprompt(dot)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

In response to

Responses

pgsql-patches by date

Next:From: David FetterDate: 2008-06-12 19:05:05
Subject: Re: SQL: table function support
Previous:From: Alex HunsakerDate: 2008-06-12 17:49:36
Subject: Re: Tentative patch for making DROP put dependency info in DETAIL

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group