Re: Row Level Security Documentation

Dean,

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> On 5 August 2017 at 10:03, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)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.

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 <literal> ... </literal> 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