Stephen, The revised patch can reproduce the original matter, as follows: ---------------- postgres=# CREATE TABLE t1 (a int, b text); CREATE TABLE postgres=# CREATE TABLE t2 (x int, y text); CREATE TABLE postgres=# GRANT select(a) on t1 to ymj; GRANT postgres=# GRANT select(x,y) on t2 TO ymj; GRANT postgres=# \c - ymj psql (8.4devel) You are now connected to database "postgres" as user "ymj". postgres=> SELECT a, y FROM t1 JOIN t2 ON a = x; NOTICE: make_var: attribute 1 added on rte(relid=16398, rtekind=0) NOTICE: make_var: attribute 1 added on rte(relid=16404, rtekind=0) NOTICE: make_var: attribute 1 added on rte(relid=0, rtekind=2) NOTICE: make_var: attribute 4 added on rte(relid=0, rtekind=2) NOTICE: pg_attribute_aclmask: t1.a required: 0002 allowed: 0002 NOTICE: pg_attribute_aclmask: t1.b required: 0002 allowed: 0000 ERROR: permission denied for relation t1 ----------------
I think it is not an essentiality whether it walks on query tree, or not. So, I also suppose putting this code on make_var(). However, it does not care the case when rte->relkind == RTE_JOIN, so it requires column-level privileges on whole of columns including unrefered ones. My suggestiong is case separations. If rte->relkind == RTE_RELATION, it can keep current behavior, as is. If rte->relkind == RTE_JOIN, it need to find the source relation recursively and marks required column. Please note that the source relation can be RTE_JOIN or others. Elsewhere, we don't need to care anymore. Thanks, Stephen Frost wrote: > Tom, et al, > > * Stephen Frost (sfr...@snowman.net) wrote: >>> applyColumnPrivs is misnamed as it's not "applying" any privileges nor >>> indeed doing much of anything directly to do with privileges. It should >>> probably be named something more like findReferencedColumns. And what's >>> with the special exception for SortGroupClause!? >> I'm not sure what the story with SortGroupClause is.. Might just have >> been a bit more difficult to do. KaiGai? > > This should be resolved now, since.. > >>> Actually, though, you probably shouldn't have applyColumnPrivsWalker at all. >>> It requires an additional traversal of the parse tree, and an additional RTE >>> search for each var, for no good reason. Can't we just mark the column >>> as referenced in make_var() and maybe a couple other places that already >>> have >>> their hands on the RTE? >> I certainly like this idea and I'll look into it, but it might take me a >> bit longer to find the appropriate places beyond make_var(). > > I've implemented and tested these suggested changes, and have removed > applyColumnPrivs, etc. It passes all the regression tests, which had a > variety of tests, so I'm reasonably happy with this. > >>> pg_attribute_aclmask seems to need a rethink. I don't like the amount >>> of policy copied-and-pasted from pg_class_aclmask, nor the fact that >>> it will fail for system attributes (attnum < 0), nor the fact that it >>> insists on looping over the attributes even if the relation privileges >>> would provide what's needed. (Indeed, you shouldn't need that "merge >>> ACLs" operation at all -- you could be ORing a couple of bitmasks >>> instead, no?) >> I'll have to think of the entry points for pg_attribute_aclmask. In >> general, we shouldn't ever get to it if the relation privileges are >> sufficient but I think it's exposed to the user at some level, where >> we would be wrong to say a user doesn't have rights on the column >> when they do have the appropriate table-level rights. Unfortunately, >> aclmask() uses information beyond the bitmasks (who granted them, >> specifically) so I don't think we can just OR the bitmasks. >> >> With regard to looping through the attributes, I could split it up into >> two functions, would that be better? A function that handles >> all-attribute cases (either 'AND'd or 'OR'd together depending on what >> the caller wants) could be added pretty easily and then >> pg_attribute_aclmask could be reverted to just handling a single >> attribute at a time (which would fix it for system attributes as >> well..). > > I've modified the code to handle system attributes but otherwise kept it > pretty much the same (with the loop and the special values to indicate > how to handle it). I looked at creating a seperate function and it > really seemed like it would be alot of code duplication.. It might > still be possible to refactor it if you'd really like. I'm open to > other thoughts or suggestions you have based on my comments above. > > Updated patch attached. > > Thanks! > > Stephen -- OSS Platform Development Division, NEC 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