Re: New patch for Column-level privileges

Stephen, I could find a strange behavior unfortunatelly. :-)
(But it is obvious one, don't worry.)

postgres=# CREATE TABLE t1 (a int, b int, c int);
CREATE TABLE
postgres=# ALTER TABLE t1 DROP COLUMN c;
ALTER TABLE
postgres=# \c - ymj
psql (8.4devel)
You are now connected to database "postgres" as user "ymj".
postgres=> SELECT 1 FROM t1;
DEBUG: pg_attribute_aclmask: t1.a required: 0002 allowed: 0000
DEBUG: pg_attribute_aclmask: t1.b required: 0002 allowed: 0000
?column?
----------
(0 rows)

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

+ AclResult
+ pg_attribute_aclcheck_all(Oid table_oid, Oid roleid, AclMode mode,
+ AclMaskHow how)
+ {
:
+ for (curr_att = 1; curr_att <= nattrs; curr_att++)
+ {
+ if (pg_attribute_aclmask(table_oid,
+ curr_att, roleid, mode, ACLMASK_ANY) != 0)
+ {
+ if (how == ACLMASK_ANY)
+ return ACLCHECK_OK;
+ }
+ else
+ {
+ if (how == ACLMASK_ALL)
+ return ACLCHECK_NO_PRIV;
+ }
+ }

But pg_attribute_aclmask() returns the required privileges set as is,
if target attribute has been already dropped, then pg_attribute_aclcheck_all()
considers it a column is allowed to select at least.

+ AclMode
+ pg_attribute_aclmask(Oid table_oid, AttrNumber att_number, Oid roleid,
+ AclMode mask, AclMaskHow how)
+ {
:
+ /* Skip dropped columns */
+ if (((Form_pg_attribute) GETSTRUCT(attTuple))->attisdropped)
+ {
+ /* if we have a detoasted copy, free it */
+ if (relacl && (Pointer) relacl != DatumGetPointer(relaclDatum))
+ pfree(relacl);
+
+ ReleaseSysCache(attTuple);
+ ReleaseSysCache(classTuple);
+
+ return mask;
+ }

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.

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.

Thanks,

Stephen Frost wrote:
> KaiGai,
>
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> The attached patch put invocations of markColumnForSelectPriv()
>> at transformJoinUsingClause() to mark those columns are used.
>
> Thanks for the update!
>
> Attached is a patch which:
>
> - incorporates KaiGai's latest patches to deal with JOINs and
> NATURAL JOINs
>
> - adds regression tests following Tom's suggestion to check
> whole-row vars in the face of column add/deletes
>
> - adds regression tests for NATURAL JOIN and successful JOINs
> with table sub-sets
>
> - reworks pg_attribute_aclmask() to remove the looping component
>
> - adds a new pg_attribute_aclcheck_all() to handle the ANY/ALL
> needs of execMain and the looping
>
> - removes special handling of system columns, they can still be
> granted/revoked, but they won't be included in ANY/ALL tests and a
> table-wide REVOKE won't affect them. After thinking about it for a
> while, I felt this was the most sensible compromise between code
> complexity, following the SQL spec, and user freedom.
>
> - split out adding column revokes for table-level commands into a
> add_col_revokes function to clean up ExecGrant_Relation a bit.
>
> - when handling table-level revokes, skips over columns which do not
> have an ACL defined, since it clearly has no effect except to force
> creation of a default ACL that's just clutter.
>
> Comments, testing, etc, most appreciated!
>
> Thanks,
>
> Stephen

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>