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@sss.pgh.pa.us>  wrote:
>> KaiGai Kohei<kaigai@ak.jp.nec.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@ak.jp.nec.com>