(2010/05/24 22:18), Robert Haas wrote: > 2010/5/24 KaiGai Kohei<kai...@ak.jp.nec.com>: >> BTW, I guess the reason why permissions on attributes are not checked here is >> that we missed it at v8.4 development. > > That's a little worrying. Can you construct and post a test case > where this results in a user-visible failure in CVS HEAD?
Sorry, after more detailed consideration, it seems to me the permission checks in RI_Initial_Check() and its fallback mechanism are nonsense. See the following commands. postgres=# CREATE USER ymj; CREATE ROLE postgres=# CREATE TABLE pk_tbl (a int primary key, b text); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "pk_tbl_pkey" for table "pk_tbl" CREATE TABLE postgres=# CREATE TABLE fk_tbl (x int, y text); CREATE TABLE postgres=# ALTER TABLE pk_tbl OWNER TO ymj; ALTER TABLE postgres=# ALTER TABLE fk_tbl OWNER TO ymj; ALTER TABLE postgres=# REVOKE ALL ON pk_tbl, fk_tbl FROM ymj; REVOKE postgres=# GRANT REFERENCES ON pk_tbl, fk_tbl TO ymj; GRANT At that time, the 'ymj' has ownership and REFERENCES permissions on both of pk_tbl and fk_tbl. In this case, RI_Initial_Check() shall return and the fallback-seqscan will run. But, postgres=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a); ERROR: permission denied for relation pk_tbl CONTEXT: SQL statement "SELECT 1 FROM ONLY "public"."pk_tbl" x WHERE "a" OPERATOR(pg_catalog.=) $1 FOR SHARE OF x" >From more careful observation of the code, the validateForeignKeyConstraint() also calls RI_FKey_check_ins() for each scanned tuples, but it also execute SELECT statement using SPI_*() interface internally. In other words, both of execution paths entirely require SELECT permission to validate new FK constraint. I think we need a new SPI_*() interface which allows to run the given query without any permission checks, because these queries are purely internal stuff, so we can trust the query is harmless. Is there any other idea? >> The attached patch provides a common checker function of DML, and modifies >> ExecCheckRTPerms(), CopyTo() and RI_Initial_Check() to call the checker >> function instead of individual ACL checks. > > This looks pretty sane to me, although I have not done a full review. > I am disinclined to create a whole new directory for it. I think the > new function should go in src/backend/catalog/aclchk.c and be declared > in src/include/utils/acl.h. If that sounds reasonable to you, please > revise and post an updated patch. > I'm afraid of that the src/backend/catalog/aclchk.c will become overcrowding in the future. If it is ugly to deploy checker functions in separated dir/files, I think it is an idea to put it on the execMain.c, instead of ExecCheckRTEPerms(). It also suggest us where the checker functions should be deployed on the upcoming DDL reworks. In similar way, we will deploy them on src/backend/command/pg_database for example? 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
