Re: New patch for Column-level privileges

KaiGai,

* KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
> Stephen, I could find a strange behavior unfortunatelly. :-)

Glad you're finding these, honestly. Better to find them and deal with
them now than after a release.

> It is a case to be failed because 'ymj' has no privileges on t1
> and its columns, but it does not raise any error.
>
> In this case, t1 has three columns but one of them has already dropped.
>
> Your pg_attribute_aclcheck_all() tries to check all the general columns
> on the given relation, and it returns ACLCHECK_OK if one of them are
> allowed and (how == ACLMASK_ANY).

Yeah, I'm not too suprised at that, honestly, it was awkward to deal
with dropped columns in pg_attribute_aclmask().

> I think pg_attribute_aclcheck_all() should skip dropped columns,
> even if it need a finding-up system cache once more.
> And, pg_attribute_aclmask() should raise an error for a request
> to dropped column, as if it could not be found.

I've implemented these changes, and updated the regression tests to
check for this. Updated patch is attached.

> BTW, what is the official reviewer's opinion?
> It seems to me most of the issues on column-level privileges are
> resolved, so it is almost ready for getting merged.

I tend to doubt Tom's had a chance to review it again yet, which is
fine, though I'm certainly hopeful the recent focus and our combined
efforts mean this patch can be included for 8.4. My personal opinion is
that it's ready for beta testing (which is kind of what this feels like
we're doing here) and barring any serious issues found during testing is
good to go for inclusion.

As for areas where there could still be some improvment, I'd love to
hear your thoughts and opinions (and others!) on how column-level
privileges are handled in ExecuteGrantStmt and into ExecGrant_Relation
and how we might improve it. I don't like the nested for() loops in
ExecuteGrantStmt, but I don't see any easy way to resolve that and keep
the SQL-required syntax. As for ExecGrant_Relation, it'd be nice if we
could shorten it somehow by either combining the relation and column
level handling into a single piece of code, or maybe refactoring it into
seperate functions which could be called from both pieces..

For both of these, I'm not sure it's really practical or would end up
being better than what's there, but that's what I'd look at next with
this patch and how it might be improved.

Thanks!

Stephen