Re: ExecutorCheckPerms() hook

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)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 go into 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 hooks at 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 comment in chkpriv_rte_perms to be simpler-
"This is only used for checking plain relation permissions, nothing
else is checked here", 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't use
'perms' much (uh, at all?) in function names, and even if we did, it'd
be redundant and not really help someone understand 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 right now, 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 (like the comment you
removed- /* We don't have table permissions, check per-column
permissions */, updated to for something like "build an RTE with the
columns referenced marked to check for necessary privileges").
Additionally, it might be worth considering if having an RTE built
farther up in DoCopy would make sense and would then be usable for
other bits in DoCopy.

- In RI_Initial_Check, why not build up an actual list of RTEs and just
call chkpriv_relation_perms once? Also, you should add 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 hook is and what it
can be used for, along with what the arguments mean, etc. Similairly,
chkpriv_relation_perms should really have 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