Re: ExecutorCheckPerms() hook

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)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.

> As I replied in the earlier message, it may be an idea to rename and change
> the definition of ExecCheckRTEPerms() without moving it anywhere.

And, again, I don't see that as a good idea at all.

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

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

Thanks,

Stephen