Re: ExecutorCheckPerms() hook

KaiGai,

* KaiGai Kohei (kaigai@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.

#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.

#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.

#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.

#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.

#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().

#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.

#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.
Thanks,
    Stephen