Re: New patch for Column-level privileges

Tom, et al,

* Stephen Frost (sfrost(at)snowman(dot)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