Tom, et al, * Tom Lane (t...@sss.pgh.pa.us) wrote: > A few random comments based on a very fast scan through the patch:
Thanks for the feedback!
> The RTE fields really ought to be bitmaps not integer lists. The
> list representation is expensive to store, copy, etc. You could use
> the same approach the HOT patch used, ie offset the indexes by
> FirstLowInvalidHeapAttributeNumber (cf pull_varattnos).
Done (was actually easier than I expected it to be).
> Patch is desperately lacking comments surrounding the altered meaning
> of ATTRIBUTE_TUPLE_SIZE, the fact that TupleDescs no longer contain
> complete copies of pg_attribute rows, etc. It might be a good idea
> to rename ATTRIBUTE_TUPLE_SIZE to ATTRIBUTE_TUPLE_FIXED_PART_SIZE
> or something like that.
Done.
> Don't like exporting allocacl() from acl.c nor having aclchk.c be so
> intimate with the internal representation of ACLs. Seems like the
> operations actually needed could be represented a bit more abstractly,
> ie copy an ACL or merge two ACLs.
Done.
> 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?
> 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 don't see anything in transformDeleteStatement to handle the
> fact that "DELETE ... WHERE foo = 42" requires select on foo.
I've fixed this (I believe).
> In the gram.y changes, don't treat CREATE differently from the other
> cases. You need a test and error in the statement execution code anyway
> for the case of a privilege type inappropriately applied to columns, and
> it's better to throw that very specific error message than the generic
> "syntax error" that bison is going to throw for CREATE (column list).
Done.
> The comment added to InsertPgAttributeTuple seems not to belong to it
> (copy/paste error?)
Fixed.
> What's the point of disallowing column privileges on a sequence? (aclcheck.c
> line 800 or so) It's not nonsensical to want to restrict what someone can do
> in "SELECT * FROM sequence".
I've removed the offending code but to be honest I'm a bit nervous that
there are other parts of the code that aren't expecting a sequence and
may have to be changed.
> 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 don't find what you've done with no_priv_msg[] to be particularly
> comfortable: if anyone ever tried to print an ACL_KIND_COLUMN error
> with the regular code path (hardly unlikely) you'd get a core dump
> due to the format wanting two %s arguments with only one supplied.
> I think the best thing is for no_priv_msg[] to include just
> + gettext_noop("permission denied for column %s"),
> and then make aclcheck_error_col() use its own error text rather than
> pulling from the array.
Done.
> Don't like naming of "Priv" node type, it's way too nonspecific.
> Also, you need outfuncs.c support for it, see comment at head of
> that file.
Done.
Updated patch attached.
Thanks!
Stephen
colprivs_2009010902.diff.gz
Description: Binary data
signature.asc
Description: Digital signature
