(2010/07/20 3:13), Robert Haas wrote: > 2010/7/12 KaiGai Kohei<kai...@ak.jp.nec.com>: >> (2010/07/10 5:53), Robert Haas wrote: >>> 2010/6/14 KaiGai Kohei<kai...@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 <kai...@ak.jp.nec.com> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers