Re: New patch for Column-level privileges

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> Please test, comment, etc.

A few random comments based on a very fast scan through the patch:

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).

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.

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.

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!?

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 don't see anything in transformDeleteStatement to handle the
fact that "DELETE ... WHERE foo = 42" requires select on foo.

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).

The comment added to InsertPgAttributeTuple seems not to belong to it
(copy/paste error?)

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".

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 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.

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.

regards, tom lane