Re: ExecutorCheckPerms() hook

KaiGai,

* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
> The attached patch is a revised one for DML permission checks.

This is certainly alot better.

> ToDo:
> - makeRangeTblEntry() stuff to allocate a RTE node with given parameter
>   is not yet.

I'd certainly like to see the above done, or to understand why it can't
be if that turns out to be the case.

A couple of other comments, all pretty minor things:

- I'd still rather see the hook itself in another patch, but given that we've determined that none of this is going to
gointo 9.0, it's not as big a deal. 

- The hook definition in aclchk.c should really be at the top of that file.  We've been pretty consistant about putting
hooksat the top of files instead of deep down in the file, this should also follow that scheme. 

- Some of the comments at the top of chkpriv_rte_perms probably make sense to move up to where it's called from
execMain.c. Specifically, the comments about the other RTE types (function, join, subquery). I'd probably change the
commentin chkpriv_rte_perms to be simpler- "This is only used for checking plain relation permissions, nothing else is
checkedhere", and also have that same comment around chkpriv_relation_perms, both in aclchk.c and in acl.h. 

- I'd move chkpriv_relation_perms above chkpriv_rte_perms, it's what we expect people to use, after all.

- Don't particularly like the function names.  How about relation_privilege_check?  Or rangetbl_privilege_check?  We
don'tuse 'perms' much (uh, at all?) in function names, and even if we did, it'd be redundant and not really help
someoneunderstand what the function is doing. 

- I don't really like having 'abort' as the variable name for the 2nd argument.  I'm not finding an obvious convention
rightnow, but maybe something like "error_on_failure" instead? 

- In DoCopy, some comments about what you're doing there to set up for calling chkpriv_relation_perms would be good
(likethe comment you removed- /* We don't have table permissions, check per-column permissions */, updated to for
somethinglike "build an RTE with the columns referenced marked to check for necessary privileges").   Additionally, it
mightbe worth considering if having an RTE built farther up in DoCopy would make sense and would then be usable for
otherbits in DoCopy. 

- In RI_Initial_Check, why not build up an actual list of RTEs and just call chkpriv_relation_perms once?  Also, you
shouldadd comments there, again, about what you're doing and why.  If you can use another function to build the actual
RTE,this will probably fall out more sensibly too. 

- Have you checked if there are any bad side-effects from calling ri_FetchConstraintInfo before doing the permissions
checking?

- The hook in acl.h should be separated out and brought to the top and documented independently as to exactly where the
hookis and what it can be used for, along with what the arguments mean, etc.  Similairly, chkpriv_relation_perms should
reallyhave a short comment for it about what it's for.  Something more than 'security checker function'. 

All pretty minor things that I'd probably just fix myself if I was going
to be committing it (not that I have that option ;).
Thanks,
    Stephen