Re: [PATCH] remove redundant ownership checks

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.

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