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