SEO

Greetings,

This really probably should have gone to -hackers rather than just back
to -patches, so here it is. Comments welcome.

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> I'm not sure where we go from here. Your GSOC student has disappeared,
> right? Is anyone else willing to take up the patch and work on it?

I'm willing to take it up and work on it. I'm very interested in having
column-level privileges in PG. I also have some experiance in the
gram.y and ACL areas already that should make things go quickly. If
anyone else is interested/has resources, please let me know.

> Admittedly the patch's syntax is more logical (especially if you
> consider the possibility of multiple tables) but I don't think we
> can go against the spec. This problem invalidates the gram.y changes
> and a fair amount of what was done in aclchk.c.

Agreed, we need to use the SQL spec syntax.

> 2. The checking of INSERT/UPDATE permissions has been moved to a
> completely unacceptable time/place, namely during parse analysis instead
> of at the beginning of execution. This is unusable for prepared
> queries, for example, and it also fails to apply permission checking
> properly for UPDATEs of inheritance trees (only the parent would get
> checked). This seems not very simple to fix :-(. By the time the plan
> gets to the executor it is not clear which columns were actually
> specified as targets by the user and which were filled in as defaults by
> the rewriter or planner. One possible solution is to add a flag field
> to TargetEntry to carry the information forward.

I'll look into this, I liked the bitmap idea, personally.

> Some other points that need to be considered:
>
> >> 1. The execution of GRANT/REVOKE for column privileges. Now only
> >> INSERT/UPDATE/REFERENCES privileges are supported, as SQL92 specified.
> >> SELECT privilege is now not supported.
>
> Well, SQL99 has per-column SELECT privilege, so we're already behind
> the curve here. The main problem again is to figure out a reasonable
> means for the executor to know which columns to check. TargetEntry
> markings won't help here. I thought about adding a bitmap to each
> RTE showing the attribute numbers explicitly referenced in the query,
> but I'm unsure if that's a good solution or not.
>
> I'd be willing to leave this as work to be done later, since 90% of
> the patch is just concerned with the mechanics of storing per-column
> privileges and doesn't care which ones they are exactly. But it
> needs to be on the to-do list.

I think it would be quite unfortunate to not include per-column SELECT
privileges with the initial version. It has significant uses and would
really be a pretty obvious hole in our implementation.

> What I think would be a more desirable solution is that the table ACL
> still sets the table-level INSERT or UPDATE privilege bit as long as
> you have any such privilege. In the normal case where no per-column
> privilege has been granted, the per-column attacl fields all remain
> NULL and that's all you need. As soon as any per-column GRANT or
> REVOKE is issued against a table, expand out the per-column ACLs to
> match the previous table-level state, and then apply the per-column
> changes. I think you'd need an additional pg_class flag column,
> say "relhascolacls", to denote whether this has been done --- then
> privilege checking would know it only needs to look at the column
> ACLs when this field is set.

I agree with this approach and feel it's alot cleaner as well as faster.
We definitely don't want to make permission checking take any more time
than it absolutely has to.

> With this scheme we don't need per-column entries in pg_shdepend,
> we can just reference the table-level bits as before. REVOKE would have
> the responsibility of getting rid of per-column entries, if any, as a
> followup to revoking per-table entries during a DROP USER operation.

Doesn't sound too bad.

> Something else that needs to be thought about is whether system columns
> have privileges or not. The patch seems to be assuming "not" in some
> places, but at least for SELECT it seems like this might be sensible.
> Also, you can already do COPY TO the OID column if any, so even without
> any future extensions it seems like we've got the issue in front of us.

I certainly feel we should be able to have per-column privileges on
system columns, though we should only use them were appropriate, of
course.

> One other mistake I noted was that the version checks added in pg_dump
> and psql are ">= 80300", which of course is obsolete now.

That one's pretty easy to handle.

Thanks,

Stephen

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFIIPt4rzgMPqB3kigRAp9YAJ9O1E5Br8clMgU6hELUL8 GkDQy2lwCfQlbF
g/3rx0UOK+q/bbAXZc2j92g=
=yuNs
-----END PGP SIGNATURE-----

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> What I think would be a more desirable solution is that the table ACL
>> still sets the table-level INSERT or UPDATE privilege bit as long as
>> you have any such privilege. ...

> I agree with this approach and feel it's alot cleaner as well as faster.
> We definitely don't want to make permission checking take any more time
> than it absolutely has to.

It occurs to me that there's something else to be thought about here.
Given a table against which some per-column GRANTs/REVOKEs have been
issued, what is the proper privilege state for a newly added column?
I'm feeling too lazy right now to try to deconstruct what the spec
says about it, if it says anything. However, I'm pretty sure that the
patch-as-given doesn't behave very reasonably (or backwards compatibly)
on the point: it's going to end up with no privileges on the new column,
whereas our historical behavior is that the new column is accessible to
anyone with table-level privileges.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers