(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

Reply via email to