Re: [HACKERS] Row Level Security Documentation
Dean, * Dean Rasheed (dean.a.rash...@gmail.com) wrote: > On 26 September 2017 at 00:42, Stephen Frost wrote: > > That's a relatively minor point, however, and I do feel that this patch > > is a definite improvement. Were you thinking of just applying this for > > v10, or back-patching all or part of it..? > > I was planning on back-patching it to 9.5, taking out the parts > relating the restrictive policies as appropriate. Currently the 9.5 > and 9.6 docs are identical, as are 10 and HEAD, and 9.5/9.6 only > differs from 10/HEAD in a couple of places where they mention > restrictive policies. IMO we should stick to that, making any > improvements available in the back-branches. I was also thinking the > same about the new summary table, although I haven't properly reviewed > that yet. Makes sense to me. +1 Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Row Level Security Documentation
On 26 September 2017 at 00:42, Stephen Frost wrote: > * Dean Rasheed (dean.a.rash...@gmail.com) wrote: >> Attached is a proposed patch... > > I've taken a look through this and generally agree with it. Thanks for looking at this. > the bits inside ... tags should be > consistently capitalized (you had one case of 'All' that I noticed) Yes, that's a typo. Will fix. > I wonder if it'd be better to have the "simple" description *first* > instead of later on, eg, where you have "Thus the overall result is > that" we might want to try and reword things to decribe it as "Overall, > it works thusly, ..., and the specifics are ...". OK, that makes sense. I'll try it that way round. > That's a relatively minor point, however, and I do feel that this patch > is a definite improvement. Were you thinking of just applying this for > v10, or back-patching all or part of it..? I was planning on back-patching it to 9.5, taking out the parts relating the restrictive policies as appropriate. Currently the 9.5 and 9.6 docs are identical, as are 10 and HEAD, and 9.5/9.6 only differs from 10/HEAD in a couple of places where they mention restrictive policies. IMO we should stick to that, making any improvements available in the back-branches. I was also thinking the same about the new summary table, although I haven't properly reviewed that yet. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Row Level Security Documentation
Dean, * Dean Rasheed (dean.a.rash...@gmail.com) wrote: > On 5 August 2017 at 10:03, Fabien COELHO wrote: > > Patch applies cleanly, make html ok, new table looks good to me. > > So I started looking at this patch, but before even considering the > new table proposed, I think there are multiple issues that need to be > resolved with the current docs: > > Firstly, there are 4 separate places in the current CREATE POLICY docs > that say that multiple policies are combined using OR, which is of > course no longer correct, since PG10 added RESTRICTIVE policy support. Ah, indeed, you're right there, those should have been updated to indicate that it was the default or perhaps clarify the difference, but I agree that doing so all over the place isn't ideal. > In fact, it wasn't even strictly correct in 9.5/9.6, because if > multiple different types of policy apply to a single command (e.g. > SELECT and UPDATE policies, being applied to an UPDATE command), then > they are combined using AND. Rather than trying to make this correct > in 4 different places, I think there should be a single new section of > the docs that explains how multiple policies are combined. This will > also reduce the number of places where the pre- and post-v10 docs > differ, making them easier to maintain. Agreed, that makes a lot of sense to me. > The proposed patch distinguishes between UPDATE/DELETE commands that > have WHERE or RETURNING clauses, and those that don't, claiming that > the former require SELECT permissions and the latter don't. That's > based on a couple of statements from the current docs, but it's not > entirely accurate. For example, "UPDATE foo SET a=NULL WHERE true > RETURNING now()" does not require SELECT permissions despite having > both WHERE and RETURNING clauses (OK, I admit that's a rather > contrived example, but still...), and conversely (a more realistic > example) "UPDATE foo SET a=b+c" does require SELECT permissions > despite not having WHERE or RETURNING clauses. I think we need to try > to be more precise about when SELECT permissions are required. Agreed. > In the notes section, there is a note about there needing to be at > least one permissive policy for restrictive policies to take effect. > That's well worth pointing out, however, I fear that this note is > buried so far down the page (amongst some other very wordy notes) that > readers will miss it. I suggest moving it to the parameters section, > where permissive and restrictive policies are first introduced, > because it's a pretty crucial fact to be aware of when using these new > types of policy. Ok. > Attached is a proposed patch to address these issues, along with a few > other minor tidy-ups. I've taken a look through this and generally agree with it. I'll note that the bits inside ... tags should be consistently capitalized (you had one case of 'All' that I noticed) and I wonder if it'd be better to have the "simple" description *first* instead of later on, eg, where you have "Thus the overall result is that" we might want to try and reword things to decribe it as "Overall, it works thusly, ..., and the specifics are ...". That's a relatively minor point, however, and I do feel that this patch is a definite improvement. Were you thinking of just applying this for v10, or back-patching all or part of it..? For my 2c, at least, I'm pretty open to clarifying things in the back-branches (and we have technically had restrictive policies for a while, they just required using an extension, so even those pieces are relevant for older versions, but might need additional caveats...). Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Row Level Security Documentation
On 5 August 2017 at 10:03, Fabien COELHO wrote: > Patch applies cleanly, make html ok, new table looks good to me. > So I started looking at this patch, but before even considering the new table proposed, I think there are multiple issues that need to be resolved with the current docs: Firstly, there are 4 separate places in the current CREATE POLICY docs that say that multiple policies are combined using OR, which is of course no longer correct, since PG10 added RESTRICTIVE policy support. In fact, it wasn't even strictly correct in 9.5/9.6, because if multiple different types of policy apply to a single command (e.g. SELECT and UPDATE policies, being applied to an UPDATE command), then they are combined using AND. Rather than trying to make this correct in 4 different places, I think there should be a single new section of the docs that explains how multiple policies are combined. This will also reduce the number of places where the pre- and post-v10 docs differ, making them easier to maintain. In the 6th paragraph of the description section, the terminology used is mixed up and does not match the terminology used elsewhere ("command" instead of "policy" and "policy" instead of "expression"). The description of UPDATE policies lists the commands that the policy applies to (UPDATE and INSERT ... ON CONFLICT DO UPDATE), but fails to mention SELECT FOR UPDATE and SELECT FOR SHARE. The proposed patch distinguishes between UPDATE/DELETE commands that have WHERE or RETURNING clauses, and those that don't, claiming that the former require SELECT permissions and the latter don't. That's based on a couple of statements from the current docs, but it's not entirely accurate. For example, "UPDATE foo SET a=NULL WHERE true RETURNING now()" does not require SELECT permissions despite having both WHERE and RETURNING clauses (OK, I admit that's a rather contrived example, but still...), and conversely (a more realistic example) "UPDATE foo SET a=b+c" does require SELECT permissions despite not having WHERE or RETURNING clauses. I think we need to try to be more precise about when SELECT permissions are required. In the notes section, there is a note about there needing to be at least one permissive policy for restrictive policies to take effect. That's well worth pointing out, however, I fear that this note is buried so far down the page (amongst some other very wordy notes) that readers will miss it. I suggest moving it to the parameters section, where permissive and restrictive policies are first introduced, because it's a pretty crucial fact to be aware of when using these new types of policy. Attached is a proposed patch to address these issues, along with a few other minor tidy-ups. Regards, Dean create-policy-docs.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Row Level Security Documentation
Hello Rod, Patch applies cleanly, make html ok, new table looks good to me. I've turned it "Ready for Committer". Thanks! -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Row Level Security Documentation
On Thu, Jul 13, 2017 at 5:49 AM, Fabien COELHO wrote: > > Hello Rod, > > This version of the table attempts to stipulate which section of the >> process the rule applies to. >> > > The table should be referenced from the description, something like "Table > xxx summarizes the ..." > Added the below which seemed consistent with other "see something else" messages. A summary of the application of policies to a command is found in . > ISTM that it would be clearer to split the Policy column into "FOR xxx > ..." and "USING" or "WITH CHECK", and to merge the rows which have the same > "FOR xxx ..." contents, something like: > >POLICY | > ---++- > | USING | ... > FOR ALL ...++- > | WITH CHECK | ... > ---++- > FOR SELECT ... | USING | ... > > So that it is clear that only ALL & UPDATE can get both USING & WITH > CHECK. This can be done with "morerows=1" on an entry so that it spans more > rows. > Done. I couldn't figure out a morecols=1 equivalent to keep everything under the Policy heading without a full colspec. > For empty cells, maybe a dash would be clearer. Not sure. Looked cluttered to me. Tried N/A first which was even worse. -- Rod Taylor diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml index c0dfe1ea4b..52a868e65d 100644 --- a/doc/src/sgml/ref/create_policy.sgml +++ b/doc/src/sgml/ref/create_policy.sgml @@ -94,6 +94,11 @@ CREATE POLICY name ON default deny policy is assumed, so that no rows will be visible or updatable. + + + A summary of the application of policies to a command is found + in . + @@ -389,6 +394,80 @@ CREATE POLICY name ON + + Policies Applied During Statement + + + + + + + + + + Policy + SELECT + INSERT + UPDATE + DELETE + + + + + FOR ALL ... + USING + WHERE clause + RETURNING clause + WHERE and RETURNING clause + WHERE and RETURNING clause + + + WITH CHECK + + new tuple + new tuple + new tuple + + + FOR SELECT ... USING + WHERE clause + RETURNING clause + WHERE and RETURNING clause + WHERE and RETURNING clause + + + FOR INSERT ... WITH CHECK + + new tuple + + + + + FOR UPDATE ... + USING + + + WHERE clause + + + + WITH CHECK + + + new tuple + + + + FOR DELETE ... USING + + + + WHERE clause + + + + + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Row Level Security Documentation
Hello Rod, This version of the table attempts to stipulate which section of the process the rule applies to. A few comments about this patch. It applies cleanly, make html is ok. It adds a summary table which shows for each case what happens. Although the information can be guessed/infered from the text, I think that a summary table is a good thing, at least because it breaks an otherwise dense presentation. I would suggest the following changes: The table should be referenced from the description, something like "Table xxx summarizes the ..." ISTM that it would be clearer to split the Policy column into "FOR xxx ..." and "USING" or "WITH CHECK", and to merge the rows which have the same "FOR xxx ..." contents, something like: POLICY | ---++- | USING | ... FOR ALL ...++- | WITH CHECK | ... ---++- FOR SELECT ... | USING | ... So that it is clear that only ALL & UPDATE can get both USING & WITH CHECK. This can be done with "morerows=1" on an entry so that it spans more rows. For empty cells, maybe a dash would be clearer. Not sure. -- Fabien -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Row Level Security Documentation
Of course, better thoughts appear immediately after hitting the send button. This version of the table attempts to stipulate which section of the process the rule applies to. On Thu, May 11, 2017 at 8:40 PM, Rod Taylor wrote: > I think the biggest piece missing is something to summarize the giant > blocks of text. > > Attached is a table that has commands and policy types, and a "yes" if it > applies. > > -- > Rod Taylor > -- Rod Taylor diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml index 3b24e5e95e..4c997a956d 100644 --- a/doc/src/sgml/ref/create_policy.sgml +++ b/doc/src/sgml/ref/create_policy.sgml @@ -389,6 +389,72 @@ CREATE POLICY name ON + + Policies Applied During Statement + + + + Policy + SELECT + INSERT + UPDATE + DELETE + + + + + FOR ALL ... USING + WHERE clause + RETURNING clause + WHERE and RETURNING clause + WHERE and RETURNING clause + + + FOR ALL ... WITH CHECK + + new tuple + new tuple + new tuple + + + FOR SELECT ... USING + WHERE clause + RETURNING clause + WHERE and RETURNING clause + WHERE and RETURNING clause + + + FOR INSERT ... WITH CHECK + + new tuple + + + + + FOR UPDATE ... USING + + + WHERE clause + + + + FOR UPDATE ... WITH CHECK + + + new tuple + + + + FOR DELETE ... USING + + + + WHERE clause + + + + + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers