(2010/05/25 21:44), Stephen Frost wrote: > KaiGai, > > * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: >> OK, the attached patch reworks it according to the way. > > Reviewing this patch, there are a whole slew of problems. > > #1: REALLY BIG ISSUE- Insufficient comment updates. You've changed > function definitions in a pretty serious way as well as moved some code > around such that some of the previous comments don't make sense. You > have got to update comments when you're writing a patch. Indeed, the > places I see a changes in comments are when you've removed what appears > to still be valid and appropriate comments, or places where you've added > comments which are just blatently wrong with the submitted patch.
Hmm. I'll revise/add the comment around the patched code. > #2: REALLY BIG ISSUE- You've added ExecutorCheckPerms_hook as part of > this patch- don't, we're in feature-freeze right now and should not be > adding hooks at this time. The patch is intended to submit for the v9.1 development, not v9.0, isn't it? > #3: You didn't move ExecCheckRTPerms() and ExecCheckRTEPerms() to > utils/acl and instead added executor/executor.h to rt_triggers.c. > I don't particularly like that. I admit that DoCopy() already knew > about the executor, and if that were the only case outside of the > executor where ExecCheckRTPerms() was getting called it'd probably be > alright, but we already have another place that wants to use it, so > let's move it to a more appropriate place. Sorry, I'm a bit confused. It seemed to me you suggested to utilize ExecCheckRTPerms() rather than moving its logic anywhere, so I kept it here. (Was it misunderstand?) If so, but, I doubt utils/acl is the best placeholder of the moved ExecCheckRTPerms(), because the checker function calls both of the default acl functions and a optional external security function. It means the ExecCheckRTPerms() is caller of acl functions, not acl function itself, isn't it? In other words, I wonder we should categorize a function X which calls A and (optionally) B as a part of A. I agreed the checker function is not a part of executor, but it is also not a part of acl functions in my opinion. If it is disinclined to create a new directory to deploy the checker function, my preference is src/backend/utils/adt/security.c and src/include/utils/security.h . > #4: As mentioned previously, the hook (which should be added in a > separate patch anyway) makes more sense to me to be in > ExecCheckRTPerms(), not ExecCheckRTEPerms(). This also means that we > need to be calling ExecCheckRTPerms() from DoCopy and > RI_Initial_Check(), to make sure that the hook gets called. To that > end, I wouldn't even expose ExecCheckRTEPerms() outside of acl.c. Also, > there should be a big comment about not using or calling > ExecCheckRTEPerms() directly outside of ExecCheckRTPerms() since the > hook would then be skipped. I don't have any differences in preference between ExecCheckRTPerms() and ExecCheckRTEPerms(), except for DoCopy() and RI_Initial_Check() have to call the checker function with list_make1(&rte), instead of &rte. > #5: In DoCopy, you can remove relPerms and remainingPerms, but I'd > probably leave required_access up near the top and then just use it to > set rte->required_access directly rather than moving that bit deep down > into the function. OK, > #6: I havn't checked yet, but if there are other things in an RTE which > would make sense in the DoCopy case, beyond just what's needed for the > permissions checking, and which wouldn't be 'correct' with a NULL'd > value, I would set those. Yes, we're building the RTE to check > permissions, but we don't want someone downstream to be suprised when > they make a change to something in the permissions checking and discover > that a value in RTE they expected to be there wasn't valid. Even more > so, if there are function helpers which can be used to build an RTE, we > should be using them. The same goes for RI_Initial_Check(). Are you saying something like makeFuncExpr()? I basically agree. However, should it be done in this patch? > #7: I'd move the conditional if (is_from) into the foreach which is > building the columnsSet and eliminate the need for columnsSet; I don't > see that it's really adding much here. OK, > #8: When moving ExecCheckRTPerms(), you should rename it to be more like > the other function calls in acl.h Perhaps pg_rangetbl_aclcheck()? > Also, it should return an actual AclResult instead of just true/false. See the comments in #3. And, if the caller has to handle aclcheck_error(), user cannot distinguish access violation errors between the default PG permission and any other external security stuff, isn't it? Thanks, -- 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
