On 5 August 2017 at 10:03, Fabien COELHO <coe...@cri.ensmp.fr> 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