Re: Reworks of DML permission checks

(2010/07/20 3:13), Robert Haas wrote:
> 2010/7/12 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>> (2010/07/10 5:53), Robert Haas wrote:
>>> 2010/6/14 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>>>> The attached patch tries to rework DML permission checks.
>>>>
>>>> It was mainly checked at the ExecCheckRTEPerms(), but same logic was
>>>> implemented in COPY TO/FROM statement and RI_Initial_Check().
>>>>
>>>> This patch tries to consolidate these permission checks into a common
>>>> function to make access control decision on DML permissions. It enables
>>>> to eliminate the code duplication, and improve consistency of access
>>>> controls.
>>>
>>> This patch is listed on the CommitFest page, but I'm not sure if it
>>> represents the latest work on this topic.  At a minimum, it needs to
>>> be rebased.
>>>
>>> I am not excited about moving ExecCheckRT[E]Perms to some other place
>>> in the code.  It seems to me that will complicate back-patching with
>>> no corresponding advantage.  I'd suggest we not do that.    The COPY
>>> and RI code can call ExecCheckRTPerms() where it is. Maybe at some
>>> point we will have a grand master plan for how this should all be laid
>>> out, but right now I'd prefer localized changes.
>>>
>>
>> OK, I rebased and revised the patch not to move ExecCheckRTPerms()
>> from executor/execMain.c.
>> In the attached patch, DoCopy() and RI_Initial_Check() calls that
>> function to consolidate dml access control logic.
> 
> This patch contains a number of copies of the following code:
> 
> +               {
> +                       if (ereport_on_violation)
> +                               aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS,
> +
> get_rel_name(relOid));
> +                       return false;
> +               }
> 
> What if we don't pass ereport_on_violation down to
> ExecCheckRTEPerms(), and just have it return a boolean?  Then
> ExecCheckRTPerms() can throw the error if ereport_on_violation is
> true, and return false otherwise.
> 
All the error messages are indeed same, so it seems to me fair enough.

As long as we don't need to report the error using aclcheck_error_col(),
instead of aclcheck_error(), this change will keep the code simple.
If it is preferable to show users the column-name in access violations,
we need to raise an error from ExecCheckRTEPerms().

> With this patch, ri_triggers.c emits a compiler warning, apparently
> due to a missing include.
> 
Oh, sorry, I'll fix it soon.

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