Re: RangeVarGetRelid()

On Thu, Nov 17, 2011 at 10:48 PM, Noah Misch <noah(at)leadboat(dot)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 <alvherre(at)commandprompt(dot)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