Re: [PATCH] remove redundant ownership checks

2009/12/22 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> (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.

That's somewhat separate from the point I was making, but it's a good
point all the same.

...Robert