Tom, et al, * Tom Lane ([EMAIL PROTECTED]) 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
Description: Digital signature