On Thu, Nov 17, 2011 at 10:48 PM, Noah Misch <n...@leadboat.com> wrote: > On Thu, Nov 17, 2011 at 08:59:58PM -0500, Robert Haas wrote: >> On Thu, Nov 17, 2011 at 4:12 PM, Alvaro Herrera <alvhe...@commandprompt.com> >> wrote: >> > Excerpts from Robert Haas's message of jue nov 17 17:51:06 -0300 2011: >> >> The trouble is, I'm not quite sure how to do that. ?It seems like >> >> permissions checks and lock-the-heap-for-this-index should be done in >> >> RangeVarGetRelid() just after the block that says "if (retry)" and >> >> just before the block that calls LockRelationOid(). ?That way, if we >> >> end up deciding we need to retry the name lookup, we'll retry all that >> >> other stuff as well, which is exactly right. ?The difficulty is that >> >> different callers have different needs for what should go in that >> >> space, to the degree that I'm a bit nervous about continuing to add >> >> arguments to that function to satisfy what everybody needs. ?Maybe we >> >> could make most of them Booleans and pass an "int flags" argument. >> >> Another option would be to add a "callback" argument to that function >> >> that would be called at that point with the relation, relId, and >> >> oldRelId as arguments. ?Alternatively, we could just resign ourselves >> >> to duplicating the loop in this function into each place in the code >> >> that has a special-purpose requirement, but the function is complex >> >> enough to make that a pretty unappealing prospect. >> > >> > I'm for the callback. > >> Having now written the patch, I'm convinced that the callback is in >> fact the right choice. It requires only very minor reorganization of >> the existing code, which is always a plus. > > +1 > > How about invoking the callback earlier, before "if (lockmode == NoLock)"? > That way, a REINDEX TABLE that blocks against an ALTER TABLE OWNER TO will > respect the new owner. Also, the callback will still get used in the NoLock > case. Callbacks that would have preferred the old positioning can check relId > == oldRelId to distinguish.
Hmm, I guess that would work. >> --- a/src/include/catalog/namespace.h >> +++ b/src/include/catalog/namespace.h >> @@ -47,9 +47,15 @@ typedef struct OverrideSearchPath >> bool addTemp; /* implicitly prepend temp >> schema? */ >> } OverrideSearchPath; >> >> +typedef void (*RangeVarGetRelidCallback)(const RangeVar *relation, Oid >> relId, >> + Oid oldRelId); >> >> -extern Oid RangeVarGetRelid(const RangeVar *relation, LOCKMODE lockmode, >> - bool missing_ok, bool nowait); >> +#define RangeVarGetRelid(relation, lockmode, missing_ok, nowait) \ >> + RangeVarGetRelidExtended(relation, lockmode, missing_ok, >> nowait, NULL) >> + >> +extern Oid RangeVarGetRelidExtended(const RangeVar *relation, >> + LOCKMODE lockmode, bool >> missing_ok, bool nowait, >> + RangeVarGetRelidCallback >> callback); > > I like the split of RangeVarGetRelidExtended from RangeVarGetRelid. Shall we > also default missing_ok=false and nowait=false for RangeVarGetRelid? I thought about that, but just did it this way for now to keep the patch small for review purposes. nowait = true is only used in one place, so it probably makes sense to default it to false in the non-extended version. But there are a number of callers who want missing_ok = true behavior, so I'm inclined not to think that one should be available in the non-extended version. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers