On 5 August 2017 at 10:03, Fabien COELHO <coelho@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers