Re: New patch for Column-level privileges

Markus Wanner wrote:
> Hello Stephen,
>
> Stephen Frost wrote:
>> ..in the attached patch.
>
> Thanks, I've reviewed this patch again.
>
>> Please find attached an updated patch for column-level privileges
>> which incorporates Alvaro's suggested changes and is updated to the
>> latest CVS HEAD.
>
> Cool, applies cleanly and compiles without any error or warning on my
> Debian box. Regression tests pass fine as well.
>
>> Regression tests have been added as well as
>> documentation (though this could probably be improved).
>
> Sorry I didn't get around writing docu. Looks good so far. Some more
> hints on its usage wouldn't hurt, especially because the error messages
> aren't overly verbose ('permission denied..' doesn't tell you much).
>
>> Currently,
>> column-level privileges are not honored when JOINs are involved (you
>> must have the necessary table-level privileges, as you do today). It
>> would really be great to have that working and mainly involves
>> modifying the rewriter to add on to the appropriate range table column
>> list entries the columns which are used in the joins and output from
>> joins.
>
> Understood. Do you plan to work on that? Or do you think the patch
> should go into 8.4 even without support for JOINs?

I tried to check the latest Column-level privileges patch.

This trouble related to JOINs is come from inappropriate handling
of rte->cols_sel list, in my opinion.
The list is constructed at scanRTEForColumn() and expandRelation().
However, scanRTEForColumn() appends an appeared attribute number
even if given RangeTblEntry is RTE_JOIN, and expandRelation() appends
all the attribute number contained within the given relation.

I think your design is basically fine, so the issue is minor one.
But it is necessary to pick up carefully what columns are really
accessed.

Here is one proposition.
Is it possible to implement a walker function to pick up appeared
columns and to chain them on rte->cols_sel/cols_mod?
In this idea, columns in Query->targetList should be chained on
rte->cols_mod, and others should be chained on rte->cols_sel.

Here is an example to pick up all appeared relation:
http://code.google.com/p/sepgsql/source/browse/trunk/sepgsql/src/backend/security/rowacl/rowacl.c#30

If you don't have enough availability, I'll be able to do it within
a few days.

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