On Thu, Nov 24, 2011 at 10:54:50AM -0500, Robert Haas wrote:
> All right, here's an updated patch, and a couple of follow-on patches.

Your committed patch looks great overall.  A few cosmetic points:

> --- a/src/backend/catalog/namespace.c
> +++ b/src/backend/catalog/namespace.c

> @@ -309,6 +313,19 @@ RangeVarGetRelid(const RangeVar *relation, LOCKMODE 
> lockmode, bool missing_ok,
>               }
>  
>               /*
> +              * Invoke caller-supplied callback, if any.
> +              *
> +              * This callback is a good place to check permissions: we 
> haven't taken
> +              * the table lock yet (and it's really best to check 
> permissions before
> +              * locking anything!), but we've gotten far enough to know what 
> OID we
> +              * think we should lock.  Of course, concurrent DDL might 
> things while

That last sentence needs a word around "might things".

> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c

> @@ -767,15 +775,13 @@ RemoveRelations(DropStmt *drop)
>                */
>               AcceptInvalidationMessages();

The above call can go away, now.

> @@ -843,6 +804,74 @@ RemoveRelations(DropStmt *drop)
>  }
>  
>  /*
> + * Before acquiring a table lock, check whether we have sufficient rights.
> + * In the case of DROP INDEX, also try to lock the table before the index.
> + */
> +static void
> +RangeVarCallbackForDropRelation(const RangeVar *rel, Oid relOid, Oid 
> oldRelOid,
> +                                                             void *arg)
> +{
> +     HeapTuple       tuple;
> +     struct DropRelationCallbackState *state;
> +     char            relkind;
> +     Form_pg_class classform;
> +
> +     state = (struct DropRelationCallbackState *) arg;
> +     relkind = state->relkind;
> +
> +     /*
> +      * If we previously locked some other index's heap, and the name we're
> +      * looking up no longer refers to that relation, release the now-useless
> +      * lock.
> +      */
> +     if (relOid != oldRelOid && OidIsValid(state->heapOid))
> +     {
> +             UnlockRelationOid(state->heapOid, AccessExclusiveLock);
> +             state->heapOid = InvalidOid;
> +     }
> +
> +     /* Didn't find a relation, so need for locking or permission checks. */

That sentence needs a word around "so need".

Thanks,
nm

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to