Re: [PATCH] remove redundant ownership checks

(2009/12/21 12:53), Robert Haas wrote:
> On Thu, Dec 17, 2009 at 7:19 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>>> [ patch to remove EnableDisableRule's permissions check ]
>>
>> I don't particularly like this patch, mainly because I disagree with
>> randomly removing permissions checks without any sort of plan about
>> where they ought to go.
>
> So where ought they to go?
>
>> If we're going to start moving these checks around we need a very
>> well-defined notion of where permissions checks should be made, so that
>> everyone knows what to expect. I have not seen any plan for that.
>
> This seems to me to get right the heart of the matter. When I
> submitted my machine-readable explain patch, you critiqued it for
> implementing half of an abstraction layer, and it seems to me that our
> current permissions-checking logic has precisely the same issue. We
> consistently write code that starts by checking permissions and then
> moves right along to implementing the action. Those two things need
> to be severed. I see two ways to do this. Given a function that (A)
> does some prep work, (B) checks permissions, and (C) performs the
> action, we could either:
>
> 1. Make the existing function do (A) and (B) and then call another
> function to do (C), or
> 2. Make the existing function do (A), call another function to do (B),
> and then do (C) itself.
>
> I'm not sure which will work better, but I think making a decision
> about which way to do it and how to name the functions would be a big
> step towards having a coherent plan for this project.

We have mixed policy in the current implementation.

The point is what database object shall be handled in this function.

If we consider a rewrite rule as a database object, not a property of
the relation, it seems to me a correct manner to apply permission checks
in the EnableDisableRule(), because it handles a given rewrite rule.

If we consider a rewrite rule as a property of a relation, not an independent
database object, it seems to me we should apply permission checks in ATPrepCmd()
which handles a relation, rather than EnableDisableRule().

My patch stands on the later perspective, because pg_rewrite entries don't
have its own ownership and access privileges, and it is always owned by
a certain relation.

Thanks,

> A related issue is where parse analysis should be performed. We're
> not completely consistent about this right now. Most of it seems to
> be done by code in the "parser" directory, but there are several
> exceptions, including DefineRule().
>
> ...Robert

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>