From: | Noah Misch <noah(at)leadboat(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)commandprompt(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: RangeVarGetRelid() |
Date: | 2011-12-05 07:09:20 |
Message-ID: | 20111205070920.GD10035@tornado.leadboat.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Heikki Linnakangas | 2011-12-05 08:53:40 | Detecting loss of client connection (was Re: Bug in walsender when calling out to do_pg_stop_backup (and others?)) |
Previous Message | Tomas Vondra | 2011-12-05 00:16:20 | Re: why local_preload_libraries does require a separate directory ? |