Re: column level privileges - Mailing list pgsql-patches
From | Tom Lane |
---|---|
Subject | Re: column level privileges |
Date | |
Msg-id | 26736.1210118069@sss.pgh.pa.us Whole thread Raw |
In response to | column level privileges (Andrew Dunstan <andrew@dunslane.net>) |
Responses |
Re: column level privileges
Re: column level privileges Re: column level privileges |
List | pgsql-patches |
Andrew Dunstan <andrew@dunslane.net> writes: >> [ column privileges patch ] I looked over this patch with the hope of applying it, but soon despaired. It needs a great deal more work than I am willing to put into it during commitfest. There are two absolutely critical must-fix problems: 1. The syntax implemented by the patch for GRANT and REVOKE has nothing to do with that specified by the standard. The patch does, eg, GRANT INSERT ON TABLE foo (col1, col2) TO somebody but unless I have lost my ability to read BNF, what the spec demands is GRANT INSERT (col1, col2) ON TABLE foo TO somebody 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. 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. 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. >> 1.1 Add a column named 'attrel' in pg_attribute catalog to store >> column privileges. Now all column privileges are stored, no matter >> whether they could be implied from table-level privilege. What this actually means, but doesn't say, is that there's no table-level representation of INSERT/UPDATE privilege any more at all. I think this is a pretty fundamental design error. In the first place it bloats pg_attribute with data that's entirely redundant for the "typical" case where per-column privileges aren't used. In the second place it slows privilege checking for the typical case, since instead of one check for the relation you have to do one for each attribute. There are some other problems too, like having to extend pg_shdepend to include an objsubid column, and some other places where the patch has to do awkward things because it's now lacking table-level information about privilege checks. 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. 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. 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. One other mistake I noted was that the version checks added in pg_dump and psql are ">= 80300", which of course is obsolete now. 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? regards, tom lane
pgsql-patches by date: