(2009/12/21 12:53), Robert Haas wrote: > On Thu, Dec 17, 2009 at 7:19 PM, Tom Lane<t...@sss.pgh.pa.us> wrote: >> KaiGai Kohei<kai...@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 <kai...@ak.jp.nec.com> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
