Re: ExecutorCheckPerms() hook

(2010/05/25 10:27), Stephen Frost wrote:
> * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>> We have two options; If the checker function takes the list of RangeTblEntry,
>> it will be comfortable to ExecCheckRTPerms(), but not DoCopy(). Inversely,
>> if the checker function takes arguments in my patch, it will be comfortable
>> to DoCopy(), but ExecCheckRTPerms().
>>
>> In my patch, it takes 6 arguments, but we can reference all of them from
>> the given RangeTblEntry. On the other hand, if DoCopy() has to set up
>> a pseudo RangeTblEntry to call checker function, it entirely needs to set
>> up similar or a bit large number of variables.
> 
> I don't know that it's really all that difficult to set up an RT in
> DoCopy or RI_Initial_Check().  In my opinion, those are the strange or
> corner cases- not the Executor code, through which all 'regular' DML is
> done.  It makes me wonder if COPY shouldn't have been implemented using
> the Executor instead, but that's, again, a completely separate topic.
> It wasn't, but it wants to play like it operates in the same kind of way
> as INSERT, so it needs to pick up the slack.
> 

Yes, it is not difficult to set up.
The reason why I prefer the checker function takes 6 arguments are that
DoCopy() / RI_Initial_Check() has to set up RangeTblEntry in addition to
Bitmap set, but we don't have any other significant reason.

OK, let's add a hook in the ExecCheckRTPerms().

>>>> * RI_Initial_Check()
>>
>> It seems to me the permission checks in RI_Initial_Check() is a bit ad-hoc.
> 
> I agree with this- my proposal would address this in a way whih would be
> guaranteed to be consistant: by using the same code path to do both
> checks.  I'm still not thrilled with how RI_Initial_Check() works, but
> rewriting that isn't part of this.

I agree to ignore the problem right now.
It implicitly assume the owner has SELECT privilege on the FK/PK tables,
so the minimum SELinux module also implicitly assume the client has similar
permissions on it.

>> In this case, we should execute the secondary query without permission checks,
>> because the permissions of ALTER TABLE is already checked, and we can trust
>> the given query is harmless.
> 
> I dislike the idea of providing a new SPI interfance (on the face of
> it), and also dislike the idea of having a "skip all permissions
> checking" option for anything which resembles SPI.  I would rather ask
> the question of if it really makes sense to use SPI to check FKs as
> they're being added, but we're not going to solve that issue here.

Apart from the topic of this thread, I guess it allows us to utilize
query optimization and cascaded triggers to implement FK constraints
with minimum code size.

Thanks
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>