Re: RangeVarGetRelid()

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.

> --- 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?

Thanks,
nm