Re: RangeVarGetRelid()

On Mon, Dec 19, 2011 at 10:11:23AM -0500, Robert Haas wrote:
> On Fri, Dec 16, 2011 at 10:32 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> >> I agree, but my point is that so far we have no callbacks that differ
> >> only in that detail. ?I accept that we'd probably want to avoid that.
> >
> > To illustrate what I had in mind, here's a version of your patch that has five
> > callers sharing a callback. ?The patch is against d039fd51f79e, just prior to
> > your recent commits.
>
> I don't think RenameRelation() ought to wait until we've taken the
> lock before doing this:
>
> + aclresult = pg_namespace_aclcheck(namespaceId, GetUserId(), ACL_CREATE);
> + if (aclresult != ACLCHECK_OK)
> + aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
> + get_namespace_name(namespaceId));
>
> I'm not sure how we can distinguished in a principled way between
> permissions checks that must be conducted before locking the relation
> and those that can be done afterwards. And I don't really want to
> make that distinction, anyhow.

Here's the rule I used: the pre-lock checks must prove that the user could, by
some command working as-designed, have taken a lock at least as strong as the
one we're about to acquire. How does that work out in practice? Relation
owners can always take AccessExclusiveLock by way of a DROP command, so
ownership is always sufficient as a pre-lock test.

The above namespace check is no exception; the object owner has authority to
take the AccessExclusiveLock independent of his authority to ultimately
complete the rename. (Changes arising from the recent discussions about
locking namespaces during DDL would complicate the situation in other ways.)

> But I see your point, otherwise. What I'm tempted to do is define a
> new function RangeVarGetRelidCheckOwner(RangeVar *relation, LOCKMODE
> lockmode, bool system_tables_ok) that will arrange to look up the OID,
> check that you own it, optionally check that it's not a system table,
> and acquire the specified lock. Internally that will call
> RangeVarGetRelidExtended() with a callback; the callback arg can be
> used to pass through the system_tables_ok flag.

Is the system catalog check special?

Thanks,
nm