Re: [PATCH] remove redundant ownership checks

On Sun, Dec 20, 2009 at 10:53 PM, I 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?

I have looked this over a little bit and I guess I don't see why the
lack of a grand plan for how to organize all of our permissions checks
ought to keep us from removing this one on the grounds of redundancy.
We have to attack this problem in small pieces if we're going to make
any progress, and the pieces aren't going to get any smaller than
this.

Per Tom's suggestion, I did a quick grep of the Slony source code for
EnableDisableRule and got no hits. I think the appropriate level of
concern about the possibility that third-party code might be calling
this function is to (1) add a comment to the function noting that it
is the caller's responsibility to check permissions, and (2) if we're
*really* concerned that someone might miss that, release-note it. It
is very unclear that it's worth even that much effort, though, since
we have zero evidence that there is any third-party code using this
function directly. A quick Google search on EnableDisableRule hits
mostly this thread.

With respect to cleaning up permissions more generally, it seems to me
that Stephen Frost hit the nail on the upthread when he remarked that
the current coding, where we're expecting certain functions in
tablecmds.c to be called with PART of the permissions checking done,
is fairly counterintuitive. I think it would be interesting to see if
someone could propose a refactoring that eliminates this and puts all
the permissions checking in a single, appropriate location. But ISTM
that this patch would be a subset of any such more comprehensive
patch, so that doesn't seem like a reason to hold off on applying this
either.

So I am inclined to go ahead and apply this with the addition of the
comment discussed above.

...Robert